Accept bytes as base64-encoded string by default

[flokli]

As per #611 (comment), this keeps thing symmetrical with the output, and how grpcurl handles it as well.

The old quoted literal behaviour is still accessible via --bytes-as-quoted-literals.

[codecov]

Codecov Report

Merging #631 (f7bd8d2) into master (5f3b4c5) will decrease coverage by 0.11%.
The diff coverage is 62.22%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #631      +/-   ##
==========================================
- Coverage   80.01%   79.90%   -0.11%     
==========================================
  Files          57       57              
  Lines        3858     3882      +24     
==========================================
+ Hits         3087     3102      +15     
- Misses        542      548       +6     
- Partials      229      232       +3     

[ktr0731]

Thank you! I suggested one.

[ktr0731]

What do you think about using a fallback to parse as quoted literals instead of adding a new option when the input string isn't Base64-encoded?
Current changes in this PR break backward compatibility; users who input quoted literals are affected by this behavior. I want to keep the same behavior if possible.

[flokli]

I could add a fallback - I'd parsing as base64 is not possible, try to parse as quoted literal.

IMHO, the problem is that it's not always certain - a inserted string might parse as both base64 or quoted literal.

As a user, you want to be certain how the data you send should be interpreted - that's why I added a --bytes-as-quoted-literals mode, so it needs to be explicitly selected.

Making base64 the default input encoding for bytes makes more sense, because it's consistent with the output.

[ktr0731]

As a user, you want to be certain how the data you send should be interpreted
Making base64 the default input encoding for bytes makes more sense, because it's consistent with the output.

This makes sense. I agree with your opinion.
However, Users who use Evans to fill bytes fields know the field should be inputted by quoted literals. So this behavior may cause confusion.

I think we should keep the current behavior for now, but what about breaking this behavior when releasing Evans v1.0.0?

In fact, as you said, this behavior causes a little confusion as input might be parsed as both Base64 and quoted literals so we need to clear explanation this behavior in the README or command description.

[flokli]

I was under the assumption right now (master branch) you need to specify a command line flag to parse as base64 (--bytes-as-base64 ) - it doesn't happen automatically.

I'm ok to wait merging this PR until we're happy changing behavior here.

We probably don't need to wait for 1.x though - a v0.x is usually expected to cause some breaking behaviour along the way. As long as we clearly document, it should be fine.

I saw there's no changelog in the repo. Should I add a paragraph mentioning the changed behavior, not just documenting the new state?

[ktr0731]

I was under the assumption right now (master branch) you need to specify a command line flag to parse as base64 (--bytes-as-base64 ) - it doesn't happen automatically.

Ah, the behavior I assumed is a parser feature that automatically switches (fallbacks) from Base64 to quoted literals. So the --bytes-as-base64 flag will be no longer necessary.

We probably don't need to wait for 1.x though - a v0.x is usually expected to cause some breaking behaviour along the way. As long as we clearly document, it should be fine.

Yes, Semantic Versioning specifies that. However, breaking backward compatibility gives users a bad experience. Therefore, it should be avoided if possible.

I saw there's no changelog in the repo. Should I add a paragraph mentioning the changed behavior, not just documenting the new state?

Usually, I describe new features in GitHub Releases (for example, https://github.com/ktr0731/evans/releases/tag/0.10.0). It's also good to write documentation on Evans's help messages (call --help) because some users use the auto-update feature.

[flokli]

@ktr0731 would you agree to the following design:

• --bytes-as-base64 selects base64 explicitly (no fallback)
• --bytes-as-quoted-literals selects bytes as quoted literals explicitly (no fallback)
• If neither of the two arguments are specified, try to parse as base64 first. If parsing fails, emit a warning and try parsing as literal string
• A bigger disclaimer about base64 being the new default is added to the Readme

That way, most old literal strings should still work, because it's very rare they're also "accidentially" valid base64 too. Users get warned for a transition period, and pre-1.0 the fallback could eventually be dropped.

[ktr0731]

Sorry, for the delay. I agree with your proposed design!
I believe this design has minimal impact on existing behavior.

[flokli]

No worries! I'll try to update the PR with the proposed changes.

[flokli]

@ktr0731 I updated the PR. It now attempts to fall back to the old encoding in case neither --bytes-as-base64 nor --bytes-as-quoted-literals was passed explicitly, as proposed in #631 (comment).

The tests have been extended to ensure the old behaviour is preserved, base64 without explicitly specifying the parameter, and behaviour with --bytes-as-base64 or --bytes-as-quoted-literals works.

PTAL.

[ktr0731]

LGTM!!
Thanks for your contribution 👍

[flokli]

Thanks! Can this be included in a release?

[ktr0731]

Sure. I'll create a new release soon.

[flokli]

Thanks!

[ktr0731]

Released 👉 https://github.com/ktr0731/evans/releases/tag/v0.10.11

[flokli]

Thanks!