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

[#3243] Ignore JSON comments #4834

Closed

Conversation

SeliasB
Copy link

@SeliasB SeliasB commented May 7, 2022

Fixes #3243

Changes proposed in this pull request:

  • added the possibility to ingore comments if in JSON file imported
  • the idea is to check if the checkbox is checked, if so, enable ALLOWS_COMMENTS and ALLOW_COMMENTS_YAML from JasonFactory method
  • skippable comments are { // ... , # ... , /* ... */ } format

@github-actions github-actions bot added Type: Feature Request Identifies requests for new features or enhancements. These involve proposing new improvements. JSON JSON as a data import/export format labels May 7, 2022
@wetneb wetneb changed the title Issue 3243 ignorejsoncomments [#3243] Ignore JSON comments May 8, 2022
Copy link
Sponsor Member

@wetneb wetneb left a comment

Choose a reason for hiding this comment

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

Thank you for this pull request!
I wonder about two things:

  • as a user, is there any case in which I would want to turn off this option? When is it important that ignoreComments == false? Intuitively, I feel like this option could be always left to true - and therefore not be exposed in the UI at all.
  • are there other features of JsonFactory that make the JSON parsing more forgiving, and if so, would it also make sense to turn them on systematically?

@SeliasB
Copy link
Author

SeliasB commented May 9, 2022

@wetneb

About setting the ignore comment to true, I feel like it could be always setted to skip comments. on another hand, I can double check, but I have noticed that if I set the ignore comments to true by default, the importer creates new empty rows on the viewer, so it probably creates unmanaged artifacts.
two possible reasons: I enabled comments on the parser as it has the same behaviour as the Jsonfactory or :
The way it preset at the importation and during the project creation is slightly different so it creates artifacts

I will try to set it true by default and double check if I can manage to have consistent behaviour on the viewer. if so, yes it is relevant to set it to true by default

@wetneb
Copy link
Sponsor Member

wetneb commented May 9, 2022

Sounds like a great plan, thank you!

@wetneb
Copy link
Sponsor Member

wetneb commented Jun 3, 2022

Hi @SeliasB, are you still working on this?

@SeliasB
Copy link
Author

SeliasB commented Jun 8, 2022

@wetneb Sorry, I've been in a rush,
should finish the work by the end of june !

@wetneb
Copy link
Sponsor Member

wetneb commented Jul 4, 2022

Hi @SeliasB, are you still working on this?

@wetneb
Copy link
Sponsor Member

wetneb commented Jul 12, 2022

Closing per inactivity, but happy to reopen if activity resumes.

@thadguidry
Copy link
Member

Thank you for this pull request! I wonder about two things:

  • as a user, is there any case in which I would want to turn off this option? When is it important that ignoreComments == false? Intuitively, I feel like this option could be always left to true - and therefore not be exposed in the UI at all.
  • are there other features of JsonFactory that make the JSON parsing more forgiving, and if so, would it also make sense to turn them on systematically?

The use case of having an option was so that users could edit and use the data that is sometimes inside the comment lines. Using facets they can filter out the first X rows, and also copy/paste metadata from within comment lines into actual real row cell values as necessary. The comment lines sometimes hold partial schema annotations and metadata. That's why the option to keep the comment lines is useful, thereby letting the users data wrangle with the lines themselves and create a full project for export that includes the incorporated metadata into JSON record row headers and then export.

@wetneb Please make this a checkbox option with default OFF. We want to continue to give choices to users, not make them for them. "Smart defaults with options - always." Thanks.

@thadguidry thadguidry reopened this Mar 15, 2024
@thadguidry thadguidry closed this Mar 15, 2024
@thadguidry
Copy link
Member

ugh this is the PR !
@wetneb Perhaps I should delete those above comments from me and put all this explanation into the Issue #3243 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JSON JSON as a data import/export format Type: Feature Request Identifies requests for new features or enhancements. These involve proposing new improvements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow option for comments in Json files to be ignored during import
3 participants