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

Make quoted integer detection optional #22

Closed
de-code opened this issue Feb 14, 2019 · 5 comments
Closed

Make quoted integer detection optional #22

de-code opened this issue Feb 14, 2019 · 5 comments

Comments

@de-code
Copy link
Contributor

de-code commented Feb 14, 2019

#15 introduced automatic conversion of quoted numeric version to INTEGER type.
For my use-case I would not want that behaviour. Some identifiers using numbers but should be treated as strings. e.g. they may not just consist of digits in other files. I am generating the JSON and would generate the JSON with the corresponding type (i.e. if I wanted something to be represented as an INTEGER then I wouldn't quote it).

@bxparks
Copy link
Owner

bxparks commented Feb 14, 2019

Hi, Thanks for this pull request. I agree that a flag to turn off this inference logic inside quoted strings is useful. In addition to integers, I think it would be useful for floats and booleans as well. So I took your PR, and created a commit on top of it to generalize the flag into --quoted_values_are_strings. I also added more tests, updated the README.md, updated the the CHANGELOG.md.

I uploaded my changes to the quoted branch. You should be able to merge that into your branch, then update the PR. If you do that, I will approve and merge the combined changes. Sound good?

@de-code
Copy link
Contributor Author

de-code commented Feb 15, 2019

I agree it equally makes sense for floats and booleans. I couldn't think of a good name right away. Your version would work well for me. (I merged it into my branch)

Quoted dates and timestamps are still processed. Maybe we could mention that in the description of the flag.

@bxparks
Copy link
Owner

bxparks commented Feb 15, 2019

That's true, quoted dates, times and timestamps are still processed. I think that makes sense because there's no other way to represent those values except as quoted strings. I will update the README.md to make this more clear after I merge in your PR.

bxparks added a commit that referenced this issue Feb 15, 2019
@bxparks
Copy link
Owner

bxparks commented Feb 15, 2019

All merged and updated. I think this can be closed.

@de-code
Copy link
Contributor Author

de-code commented Feb 15, 2019

That's great, thank you!

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

No branches or pull requests

2 participants