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

Add comments when dumping #10

Merged
merged 5 commits into from
Jun 20, 2023
Merged

Add comments when dumping #10

merged 5 commits into from
Jun 20, 2023

Conversation

solaluset
Copy link
Contributor

I thought it'd be useful to have a possibility to add comments when dumping by passing comments=... to dump(s). Also I've added trailing_comma arg that adds trailing commas everywhere.

@n-takumasa
Copy link
Owner

Thanks for the suggestion.

What is your intended use-case?
Considering symmetry, load should add a function to get comments.
But I think comments are not data.

@solaluset
Copy link
Contributor Author

Exactly, comments are not data. That's why I think it shouldn't be possible to extract them. But adding them is a different thing. My program generates a config file and I want the user to see what is the meaning of some settings. You can see the code here:

https://github.com/Krutyi-4el/DandelionMusic/blob/44e34d1ce23709f6214a6d5ea3d5ff402d27078c/config/config.py#L176

Copy link
Owner

@n-takumasa n-takumasa left a comment

Choose a reason for hiding this comment

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

request changes:

  • add annotations for _get_comments
  • add error handling when any inconsistencies occur with data and comments

jsonc/__init__.py Outdated Show resolved Hide resolved
jsonc/__init__.py Outdated Show resolved Hide resolved
@n-takumasa
Copy link
Owner

I see your intent, I thin it's fine for that use-case.
However, I have made some requests to improve robustness, please consider them.

@solaluset
Copy link
Contributor Author

add error handling when any inconsistencies occur with data and comments

Not sure about this one. Should the script validate that all keys in dictionary are strs when commenting dict and ints when list?

@n-takumasa
Copy link
Owner

n-takumasa commented Jun 20, 2023

add error handling when any inconsistencies occur with data and comments

Not sure about this one. Should the script validate that all keys in dictionary are strs when commenting dict and ints when list?

I think that at least occurence a warning will help.

print(jsonc.dumps({"b": 1}, indent=2, comments={"a": "foo"}))
# {
#   "b": 1
# }

Also, when you give comments arg with no indent arg, a warning should be occur.

@solaluset
Copy link
Contributor Author

I've added warns for unused comment keys. However, it has a side effect of removing all used ones from original dictionary. I'll add copy.deepcopy() if you think it may become an issue.

@n-takumasa
Copy link
Owner

Nice work

deepcopy is necessary because I usually think that functions must not have any side effect on their arguments.

@n-takumasa n-takumasa merged commit f1c09c0 into n-takumasa:main Jun 20, 2023
@n-takumasa
Copy link
Owner

merged. thanks!

@solaluset solaluset deleted the dump-comments branch June 20, 2023 20:43
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.

2 participants