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

Allow <json repr="string"> for int types #330

Merged
merged 4 commits into from
Mar 17, 2023
Merged

Allow <json repr="string"> for int types #330

merged 4 commits into from
Mar 17, 2023

Conversation

sebastiantoh
Copy link
Contributor

@sebastiantoh sebastiantoh commented Feb 27, 2023

This PR addresses issue #273 and supersedes #291

PR checklist

  • New code has tests to catch future regressions
  • Documentation is up-to-date
  • CHANGES.md is up-to-date

@sebastiantoh sebastiantoh marked this pull request as ready for review February 27, 2023 01:43
Comment on lines +683 to +722
"json encoding int with string representation", test_encoding_int_with_string_repr;
"json encoding & decoding int with string representation", test_encoding_decoding_int_with_string_repr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have a couple of tests encoding ints as ints?

Copy link
Contributor

@cyberhuman cyberhuman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks!

@sebastiantoh
Copy link
Contributor Author

Hi @mjambon, would appreciate your review on this PR. Thank you

@sebastiantoh
Copy link
Contributor Author

@mjambon gentle ping. I'll be happy to address any concerns you may have about this PR 🙂

@mjambon
Copy link
Collaborator

mjambon commented Mar 17, 2023

@sebastiantoh sorry, I didn't realize you wanted my review specifically. I reviewed the changes, it looks good to me. Let's merge it.

I was wondering what happens to the target languages (Python, ...) that don't support int <json repr="string">. It turns out that in the case of atdpy and atdts, the annotation is ignored silently. This isn't new but it's something to improve later for these languages.

@mjambon mjambon merged commit 12ea5c5 into ahrefs:master Mar 17, 2023
@mjambon
Copy link
Collaborator

mjambon commented Mar 17, 2023

@sebastiantoh and thank you!

mjambon added a commit to mjambon/opam-repository that referenced this pull request May 12, 2023
…n-codec-runtime and atd (2.12.0)

CHANGES:

* atdgen: Annotate generated code with types to disambiguate OCaml
  classic variants (ahrefs/atd#331)
* atdpy: Support the option type more correctly so that it follows
  ATD's convention for JSON encoding. This allows compatibility with
  JSON produced by other tools of the ATD suite. The Python type,
  however, is still a nullable (`Optional`) to make things simpler for
  Python programmers. This prevents distinguishing `["Some", "None"]`
  from `"None"` which both translate to `None` in Python. (ahrefs/atd#332)
* (BREAKING) atdgen: revert default encoding of int64 values as string (ahrefs/atd#330)
* atdgen: Support `<json repr="string">` for `int` values (ahrefs/atd#330)
* atdpy: Treat default field values as expressions to evaluate each time
  they're assigned to a field. This allows the use of mutable defaults such as
  lists (ahrefs/atd#339)
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.

3 participants