-
-
Notifications
You must be signed in to change notification settings - Fork 307
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
lib: Add a standard parser option for JSON formatting #3704
lib: Add a standard parser option for JSON formatting #3704
Conversation
49e78b7
to
9bdcd76
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also replace it in v.db.select, the options are different there, so you keep the current v.db.select options and descriptions.
I would think the format should be by default required, although since it has default answer, it probably doesn't matter that much.
This is great improvement @kritibirda26. To fix the OSGeo4W CI, please update your branch from main (e.g. Update branch on GitHub or merge main locally into this branch). |
9bdcd76
to
856d40f
Compare
@petrasovaa Hi! I have updated v.db.select and also changed the option to be required by default. |
856d40f
to
6d06cfa
Compare
The tests are failing with this weird error but the build passes locally. Also, the string that is said to be invalid is actually a valid isoformat string. Any suggestions on how to debug?
https://github.com/OSGeo/grass/actions/runs/9131120968/job/25109628154?pr=3704#step:8:6469 |
A guess: it probably expects a string without the 'Z' character? |
@neteler I see, do you know if there is a local git setting I can configure to fix the datetime format for the commit timestamp? |
6d06cfa
to
e6811c2
Compare
This is failing across several PRs with repeated runs. I'm opening a separate issue to discuss this. |
What prefix for the title of the PR should be used before merging, once it is rebased? (the build issue is most probably fixed now on main). |
As a part of OSGeo#3019, JSON format support will be added to multiple modules. By default, modules output in existing plain format. If the format=json option is provided, modules will output in JSON format instead. To avoid duplication of code across several modules, define a standard parser option.
e6811c2
to
c20b39c
Compare
Co-authored-by: Nicklas Larsson <n_larsson@yahoo.com> Co-authored-by: Anna Petrasova <kratochanna@gmail.com>
Thanks all for reviewing the PR and helping fix the CI issues. Please let me know me if any other changes are needed in this PR. |
@petrasovaa previously requested changes, she might want to place an approval to overrule her review. And a prefix for the PR, so it can be classified in the release notes. I'm not sure which one to apply. |
Sorry for beeing late here. I just noticed that this new standard parser option:
Should the format option also cover other formats (CSV, shell, ...), those are currently missing, but a long list of possible formats will probably defeat the purpose of the standard paser option? Just comments you may consider... Anyway, I am glad to see this standard parser option... |
As a part of #3019, JSON format support will be added to multiple modules. By default, modules output in existing plain format. If the format=json option is provided, modules will output in JSON format instead. To avoid duplication of code across several modules, define a standard parser option.