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

Adds a JSON parsing library for tasks.json #5954

Merged
merged 8 commits into from
Sep 1, 2020
Merged

Adds a JSON parsing library for tasks.json #5954

merged 8 commits into from
Sep 1, 2020

Conversation

dan-shaw
Copy link
Contributor

Currently, comments are removed when parsing tasks.json. This PR adds a new dependency that keeps user comments.

@sean-mcmanus
Copy link
Contributor

@Colengms Was there a GitHub issue tracking the comment removal issue?

@Colengms
Copy link
Contributor

@Colengms Was there a GitHub issue tracking the comment removal issue?

@sean-mcmanus Are you referring to the issue when editing c_cpp_properties.json via the Configuration UI? I dont think so.

@dan-shaw Can this also be extended to replace existing uses of jsonc-parser? It would be nice to depend on only 1 jsonc parser.

@sean-mcmanus sean-mcmanus mentioned this pull request Aug 18, 2020
Closed
sean-mcmanus
sean-mcmanus requested changes Aug 19, 2020
View reviewed changes
Copy link
Contributor

@sean-mcmanus sean-mcmanus left a comment

Choose a reason for hiding this comment

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

I don't see anything fixed by this PR. When I have a tasks.json or c_cpp_properties.json and then trigger an action that causes one of those to be editted by our code, then the comments get overwritten still. Maybe some extra code needs to be added preserver the comments?

@sean-mcmanus
Copy link
Contributor

I don't see anything fixed by this PR. When I have a tasks.json or c_cpp_properties.json and then trigger an action that causes one of those to be editted by our code, then the comments get overwritten still. Maybe some extra code needs to be added preserver the comments?

Oh, I had the wrong branch checked out.

@sean-mcmanus sean-mcmanus reopened this Aug 19, 2020
@sean-mcmanus
Copy link
Contributor

Closing this PR was accidental, not sure how that happened.

@elahehrashedi
Copy link
Contributor

@Colengms Was there a GitHub issue tracking the comment removal issue?
This issue happens in Vs Code as well: microsoft/vscode#37138
that's good if we fix it here to preserve comments. However, if the user runs some commands such as "Tasks -> Configure Default Build Task" the comments will be removed by Vs Code.

@sean-mcmanus
Copy link
Contributor

@dan-shaw This PR seems close to getting checked in and seems to just need 3 minor changes I mentioned in my previous comments. Did you plan to make those changes or did you want us to do that?

@dan-shaw
Copy link
Contributor Author

dan-shaw commented Sep 1, 2020

@sean-mcmanus Sorry, just fixed it.

@sean-mcmanus sean-mcmanus merged commit ca777dd into microsoft:master Sep 1, 2020
@elahehrashedi elahehrashedi mentioned this pull request Sep 1, 2020
Merged
@elahehrashedi
Copy link
Contributor

C/C++ extension will keep the comments while modifying tasks.json file. However, Vs Code doesn't.
I opened an issue on Vs Code to follow up on this issue: microsoft/vscode#105885

@github-actions github-actions bot locked and limited conversation to collaborators Oct 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sean-mcmanus sean-mcmanus sean-mcmanus approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dan-shaw @sean-mcmanus @Colengms @elahehrashedi