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

refactor: use the English json as the schema #41146

Merged

Conversation

ojeytonwilliams
Copy link
Contributor

@nhcarrigan I was thinking about #41140 and I reckon this problem arose because it's not obvious to a human what donate.confirm-3 is and when it should be used instead of donate.confirm-2. That inspired me to rewrite them (confirm-monthly, confirm-one-time etc.) and then I realised I'd need to rewrite the schema too.

That leads me to this PR. Since we're trying to validate that all three languages have the same json structure, is it sufficient to validate that everything has the same structure as english?

@gitpod-io
Copy link

gitpod-io bot commented Feb 17, 2021

@naomi-lgbt
Copy link
Member

naomi-lgbt commented Feb 17, 2021

When I added this validation script, the schemas were already being used to test (through a different approach) and had to be re-written to work with this approach. I vaguely remember thinking about using the English files as the single source of truth, but thought it was a bad approach. Can't for the life of me remember why I thought that, though.

Regardless, the schemas are proving to be a bit of technical overhead that I agree we don't need (I forget to update them myself all the time). 100% in favour of removing them and using the English file as the root for testing.

When this lands, I'll update the validate-keys script to not rely on the non-existent schema files.

naomi-lgbt
naomi-lgbt previously approved these changes Feb 17, 2021
Copy link
Member

@naomi-lgbt naomi-lgbt left a comment

Choose a reason for hiding this comment

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

IDK if an approval sticks around on a draft PR, but this looks good to me.

@ojeytonwilliams ojeytonwilliams marked this pull request as ready for review February 17, 2021 16:56
@ojeytonwilliams ojeytonwilliams requested a review from a team February 17, 2021 16:56
@ojeytonwilliams
Copy link
Contributor Author

Yeah, I remember us discussing it briefly, but I can't remember what we concluded either. If this proves troublesome somehow we can always revert.

When this lands, I'll update the validate-keys script to not rely on the non-existent schema files.

You can pop that onto this PR if you reckon it fits.

@raisedadead
Copy link
Member

I agree, Nick you should update this PR to update the validation script so it stays together.

@naomi-lgbt
Copy link
Member

Just took a look at this - the validate-keys script was already running against the English translations.json file.

However, I'm going to push an update to validate the other json files as well.

Validates the keys in all of the translation JSON objects. Note that
this will return some false positives for dynamic keys (like the
superblock intros). This is okay as the script is not meant to be
used as blocking CI, but only to help us clean up keys as we update
our client/API.
Copy link
Member

@naomi-lgbt naomi-lgbt left a comment

Choose a reason for hiding this comment

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

Not sure if I qualify, but still looks good to me

@raisedadead raisedadead merged commit e7aa1c5 into freeCodeCamp:main Feb 18, 2021
@ojeytonwilliams ojeytonwilliams deleted the refactor/english-json-as-schema branch February 18, 2021 07:30
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.

None yet

3 participants