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

check-json doesn't detect duplicate keys #554

Closed
adamchainz opened this issue Feb 4, 2021 · 6 comments · Fixed by #558
Closed

check-json doesn't detect duplicate keys #554

adamchainz opened this issue Feb 4, 2021 · 6 comments · Fixed by #558

Comments

@adamchainz
Copy link

The YAML check does this since #351 but since the json module's default behaviour is to ignore duplicates, duplicates are still possible in JSON. I think a small object_pairs_hook can be used to detect duplicates?

@MiHarsh
Copy link

MiHarsh commented Feb 7, 2021

Hello Sir, I didn't understand exactly what we have to do. Do we need to add something to check for duplicates? If duplicate values found then we need to raise error? or something else? Please help !!

Do we need to return retval = 1 if found duplicates,so as to denote error in the file?

@AdityaKhursale
Copy link
Contributor

Hi @asottile

I opened PR #558 for this issue. Did local verification for +ve/-ve test scenarios.

However, CI is failing. Could you please help review and let me know what need to be changed?

This is my first contribution in this repo. I am not very familiar with CI checks here and also, fairly new to open source.
Would appreciate your help.

Following is the local verification -

image

@asottile
Copy link
Member

I commented on the PR 👍

@adamchainz
Copy link
Author

Thanks @AdityaKhursale @asottile 👍

@marcindulak
Copy link

Can this check be made optional, so there is an option to restore the original check-json behavior where duplicate keys are allowed? @asottile

@asottile
Copy link
Member

I don't think so, no -- duplicate keys are a mistake

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

5 participants