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

feat: support comments in JSON #571

Merged
merged 4 commits into from Jan 8, 2021
Merged

feat: support comments in JSON #571

merged 4 commits into from Jan 8, 2021

Conversation

JustinBeckwith
Copy link
Collaborator

@JustinBeckwith JustinBeckwith commented Sep 28, 2020

Fixes #442

@pastelmind
Copy link

Is there a way I can assist this get merged?

GitHub says that a test has failed. Unfortunately, the logs have expired, so we don't know what the problem was.

@JustinBeckwith
Copy link
Collaborator Author

This is the weirdest thing. The tests always pass locally. They fail sporadically, not dependent on node version, in GitHub Actions with the error:

 1) clean
       should avoid deleting .:
     Uncaught AssertionError [ERR_ASSERTION]: The input did not match the regular expression /Unable to parse/. Input:

'Error: Error: /tmp/tmp-3007FygcrcB0qcvm/tsconfig.json\n' +
  "ENOENT: no such file or directory, open '/tmp/tmp-3007FygcrcB0qcvm/tsconfig.json'"

@pastelmind if you're interested, you could always fork the repo, checkout this branch into your own fork, and then submit a PR to try and figure things out :) Just make sure to retry the tests a few times, because the error seems to be flaky.

@JustinBeckwith
Copy link
Collaborator Author

After closer inspection.... I forgot to await a promise 🙈 @bcoe whenever you're around, this could use a gander.

@bcoe
Copy link
Contributor

bcoe commented Jan 8, 2021

@JustinBeckwith apologies for dropping this review on the ground, mind cleaning up the package-lock.json conflict, and we can work on landing.

@@ -49,6 +49,24 @@ describe('clean', () => {
});
});

it('should gracefully handle JSON with comments', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that these assertions should be async too? () => { to async () => {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope! They return a promise, since withFixtures returns a promise, which seems to be what mocha is actually looking for. All the tests in this file seem to work like that 🤷

@bcoe bcoe merged commit cb6d2ca into master Jan 8, 2021
@JustinBeckwith JustinBeckwith deleted the json5 branch January 22, 2021 22:38
nevilm-lt pushed a commit to nevilm-lt/gts that referenced this pull request Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gts clean fails with file-not-found for tsconfig.json if it has c-style comments
3 participants