Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(tracex): internally represent errors as strings #786

Merged
merged 1 commit into from
Jun 2, 2022

Conversation

bassosimone
Copy link
Contributor

@bassosimone bassosimone commented Jun 2, 2022

There are two reasons why this is beneficial:

  1. github.com/google/go-cmp is more annoying to use for comparing
    data structures when there are interfaces to compare. Sure, there's
    a recipe for teaching it to compare errors, but how about making
    the errors trivially comparable instead?

  2. if we want to send errors over the network, JSON serialization
    works but we cannot unmarshal the resulting string back to an error,
    so how about making this representation trivial to serialize (we
    are not going this now, but we need this property for websteps and
    it may be sensible to try to avoid to have duplicate code because
    of that -- measurex currently duplicates many tracex functionality
    and this is quite unfortunate because it slows development down)

Additionally, if an error is a string:

  1. we can very easily use a switch for comparing its possible
    values with "" representing the absence of errors, while it is
    more complex to do the same when using a nullable string or even
    an error (i.e., an interface)

  2. if a type is not nullable, it's easier to write safe code for
    it and we may want to refactor experiments to use the internal
    representation of measurements for more robust processing code

For all these reasons, let's internally use strings in tracex.

The overall aim here is to reduce the duplicated code between pre
and post-measurex measurements (see ooni/probe#2035).

There are two reasons why this is beneficial:

1. github.com/google/go-cmp is more annoying to use for comparing
data structures when there are interfaces to compare. Sure, there's
a recipe for teaching it to compare errors, but how about making
the errors trivially comparable instead?

2. if we want to send errors over the network, JSON serialization
works but we cannot unmarshal the resulting string back to an error,
so how about making this representation trivial to serialize (we
are not going this now, but we need this property for websteps and
it may be sensible to try to avoid to have duplicate code because
of that -- measurex currently duplicates many tracex functionality
and this is quite unfortunate because it slows development down)

Additionally, if an error is a string:

3. we cannot very easily use a switch for comparing its possible
values with "" representing the absence of errors, while it is
more complex to do the same when using a nullable string or even
and error

4. if a type is not nullable, it's easier to write safe code for
it and we may want to refactor experiments to use the internal
representation of measurements for more robust processing code

For all these reasons, let's internally use strings in tracex.

The overall aim here is to reduce the duplicated code between pre
and post-measurex measurements (see ooni/probe#2035).
@bassosimone bassosimone requested a review from hellais as a code owner June 2, 2022 08:29
@bassosimone bassosimone changed the title refactor(tracex): internally represent errors as flat strings refactor(tracex): internally represent errors as strings Jun 2, 2022
@bassosimone bassosimone merged commit 83e3167 into master Jun 2, 2022
@bassosimone bassosimone deleted the issue/2121 branch June 2, 2022 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant