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

Remove trailing commas that break the json schema #12644

Merged
merged 3 commits into from
Mar 10, 2022
Merged

Remove trailing commas that break the json schema #12644

merged 3 commits into from
Mar 10, 2022

Conversation

sowmya-hub
Copy link
Contributor

Summary of the Pull Request

Fixed #12639

PR Checklist

  • Closes #xxx
  • CLA signed. If not, go over here and sign the CLA
  • Tests added/passed
  • Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
  • Schema updated.
  • I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: Trailing commas in profiles schema #12639

Validation Steps Performed

Ran the file through a linter to check validity.

@sowmya-hub
Removed trailing commas that prevented the file from getting parsed. …
b90018d 
…Ran the file through a linter to check validity.
@ghost ghost added Area-Schema Things that have to do with the json schema. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. labels Mar 8, 2022
DHowett
DHowett requested changes Mar 9, 2022
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

It looks like this change reformats the entire file, which makes it rather difficult to review and breaks the JSON style we've been using across the repository. Would you mind scoping this change down to only the appropriate lines?

There's also a new file added to the root of the repo that we haven't figured out the purpose of.

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 9, 2022
@sowmya-hub
Copy link
Contributor Author

Thanks for doing this!

It looks like this change reformats the entire file, which makes it rather difficult to review and breaks the JSON style we've been using across the repository. Would you mind scoping this change down to only the appropriate lines?

There's also a new file added to the root of the repo that we haven't figured out the purpose of.

Thank you for pointing it out, I wasn't aware!

I'll go over the file and try to match the style according to styling used in the repository, then. Yes, I'll try to narrow the change down to those lines.

I'm also not sure where the new file came from, I'll do some more research on that and get back to you.

@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 10, 2022
@lhecker
Copy link
Member

lhecker commented Mar 10, 2022

@sowmya-hub FYI: I believe most maintainers of this project use Visual Studio Code's built-in formatter to format non-C++ files.
The good news is that the file is already formatted that way, so we only need to remove those 4 commas in this PR. 🙂

@sowmya-hub
Copy link
Contributor Author

@lhecker thanks so much for the heads-up! 🙌

It's helpful to know the built-in formatted is to be used!

I realise that the styling change came from the third-party linter I used. Since I copied the contents into the file, it got changed.

I've changed it and committed to my branch. Hopefully, it works now!

@lhecker
Copy link
Member

lhecker commented Mar 10, 2022 •
edited
Loading

@sowmya-hub If you check the top line of this PR, you'll see it still says +2,382 −2,381 unfortunately. 😕
You can restore the original contents of your file by running

git restore -s origin/main doc/cascadia/profiles.schema.json

You'll also have to remove the extra file you added:

git rm rectify-trailing-commas

If you're using Visual Studio Code as well, I can only suggest to open the command palette with Ctrl+Shift+P and search for an entry called "File: Save without Formatting", which will save the file without running your linters. If you use a different editor it will probably have a similar option somewhere.

@sowmya-hub
Copy link
Contributor Author

Oh, just saw. There was an extra line at the bottom that had surfaced thanks to the lint checks, must've been the cause.

I've run this command, neat since I didn't have to copy-paste from the original json file again!

Oh no, hadn't realised that it had shown up there(my bad, DHowett did mention it was the root of repo.) as I was searching folder-wise.

Yes, I've saved the file without formatting this time. I'm using Visual Studio Code, via Gitpod.

DHowett
DHowett approved these changes Mar 10, 2022
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Thanks so much for fixing this! I appreciate you responding to our review feedback as well 😄

@DHowett DHowett changed the title Removed trailing commas that prevented the file from getting parsed. Remove trailing commas that break the json schema Mar 10, 2022
@DHowett DHowett merged commit 78ec74a into microsoft:main Mar 10, 2022
@sowmya-hub
Copy link
Contributor Author

Thank you both so much for reviewing the changes and giving me pointers as well! 😊

@claudio-salvio
Copy link

Hello @DHowett

I am using Windows Terminal Version: 1.12.10393.0 and I still have the problem (Trailing commas in profiles schema - Issue #12639 - microsoft/terminal - https://github.com/microsoft/terminal/issues/12639) that originated this fix.

Can you please let me know if the new schema is supposed to be deployed already or how I have to do to avoid the mentioned problem?

Best regards,
Claudio Salvio

DHowett pushed a commit that referenced this pull request Apr 1, 2022
@sowmya-hub @DHowett
Remove trailing commas that break the json schema (#12644)
3dcd727 
Fixes #12639

(cherry picked from commit 78ec74a)
Service-Card-Id: 80040156
Service-Version: 1.13
@DHowett
Copy link
Member

DHowett commented Apr 1, 2022

My bad! I didn't mark this for servicing into the current active branch for the schema.

That is now done. If you can get VSCode to refresh the schema, the copy linked from $schema has been updated. 😄

@claudio-salvio
Copy link

@DHowett

Thanks for taking care of it.

The previous error message disappeared.
However now it shows a warning that says:
Draft 2020-12 schemas are not yet fully supported.
It seems to generate no problems but I mention it because I don't remember this warning appearing before.

For editing I am using VSCode Version: 1.63.2

Greetings,
Claudio Salvio

@ghost
Copy link

ghost commented Apr 19, 2022

🎉Windows Terminal Preview v1.13.1098 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Apr 19, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lhecker lhecker lhecker approved these changes

@DHowett DHowett DHowett approved these changes

Assignees
No one assigned
Labels
Area-Schema Things that have to do with the json schema. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Trailing commas in profiles schema
4 participants
@sowmya-hub @lhecker @claudio-salvio @DHowett