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.

Sign up for GitHub

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

Jump to bottom

Move to use JSONC instead of JSON for easy readability #702

Merged
merged 7 commits into from
Jan 31, 2024
Merged

Move to use JSONC instead of JSON for easy readability #702

merged 7 commits into from
Jan 31, 2024

Conversation

gab-arrobo
Copy link
Collaborator

This PR converts the JSON configuration files to JSONC to make easier to add comments

@gab-arrobo
Copy link
Collaborator Author

@amarsri28 @sureshmarikkannu @thakurajayL @badhrinathpa, any comments/suggestions about this PR? Another alternative might be to use YAML instead of JSON, however, I have not tested it. Moving to JSONC is tested and worked as expected.

@github-actions
Copy link

This pull request has been stale for 30 days and will be closed in 5 days. Comment to keep it open.

@amarsri28
Copy link
Contributor

in current code we are using " " for putting comment in json file.
Jsonc looks ok , only issue is that lot of files got modified and we need to ensure if all changes can get verified in testing or how much effort is needed to test all cosmetic changes.
did we got any requirement or issue reported by others or open source community for comments??
Lets take other reviewers opinion on this.

@github-actions
Copy link

This pull request has been stale for 30 days and will be closed in 5 days. Comment to keep it open.

@github-actions github-actions bot closed this Sep 1, 2023
@gab-arrobo gab-arrobo reopened this Sep 1, 2023
@github-actions
Copy link

github-actions bot commented Oct 2, 2023

This pull request has been stale for 30 days and will be closed in 5 days. Comment to keep it open.

@github-actions GitHub Actions
Copy link

This pull request has been stale for 30 days and will be closed in 5 days. Comment to keep it open.

@github-actions GitHub Actions
Copy link

This pull request has been stale for 30 days and will be closed in 5 days. Comment to keep it open.

@gab-arrobo gab-arrobo added enhancement New feature or request and removed stale/pr labels Dec 24, 2023
@gab-arrobo
Copy link
Collaborator Author

in current code we are using " " for putting comment in json file. Jsonc looks ok , only issue is that lot of files got modified and we need to ensure if all changes can get verified in testing or how much effort is needed to test all cosmetic changes. did we got any requirement or issue reported by others or open source community for comments?? Lets take other reviewers opinion on this.

My initial motivation for this PR was that we are already using JSONC for the CNDP configuration files and we can use the same format for the UPF. Moreover, I was just cleaning up some branches in the UPF repo and I came across this issue that Sai has opened at the end of 2021 (#370) with the same idea/improvement

thakurajayL
thakurajayL reviewed Jan 23, 2024
View reviewed changes
conf/utils.py Outdated Show resolved Hide resolved
thakurajayL
thakurajayL reviewed Jan 23, 2024
View reviewed changes
pfcpiface/config.go
if err != nil {
return Conf{}, err
}
jsonData := removeComments(string(jsoncFile))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need this?

Copy link
Collaborator Author

@gab-arrobo gab-arrobo Jan 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, we need these changes. As shown below, if we do not remove the comments from the file, there will be an "unexpected" character (/) when unmarshalling the data:

               Error Trace:    config_test.go:92
|               Error:          Received unexpected error:
|                               invalid character '/' looking for beginning of value
|               Test:           TestLoadConfigFile/all_sample_configs_must_be_valid
|               Messages:       config ../conf/upf.jsonc is not valid

Alternatively, we can revert the changes (i.e., not to use the removeComments function) and try to use a package that reads/loads JSONC files such as https://github.com/kokizzu/json5b. What are your thoughts/preference on this?

@gab-arrobo
Copy link
Collaborator Author

@thakurajayL any further comments/questions?

@gab-arrobo
Copy link
Collaborator Author

@badhrinathpa, any input/feedback bout this PR? :-)

@gab-arrobo gab-arrobo merged commit 58eedc0 into omec-project:master Jan 31, 2024
11 checks passed
@gab-arrobo gab-arrobo deleted the move-to-jsonc branch January 31, 2024 17:08
@gab-arrobo gab-arrobo mentioned this pull request Feb 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajaythakurintel ajaythakurintel ajaythakurintel approved these changes

@thakurajayL thakurajayL thakurajayL approved these changes

@badhrinathpa badhrinathpa Awaiting requested review from badhrinathpa

@sureshmarikkannu sureshmarikkannu Awaiting requested review from sureshmarikkannu

@amarsri28 amarsri28 Awaiting requested review from amarsri28

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gab-arrobo @amarsri28 @thakurajayL @ajaythakurintel