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: add support for standard-engine@15 #376

Merged
merged 9 commits into from
Feb 6, 2022

Conversation

NemoStein
Copy link
Contributor

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix
[X] New feature
[ ] Other, please explain:

What changes did you make? (Give an overview)
Added support for Standard 17

Which issue (if any) does this pull request address?
#375

Is there anything you'd like reviewers to focus on?

@welcome
Copy link

welcome bot commented Feb 4, 2022

🙌 Thanks for opening this pull request! You're awesome.

@theoludwig theoludwig linked an issue Feb 4, 2022 that may be closed by this pull request
@theoludwig theoludwig self-requested a review February 4, 2022 07:35
Copy link
Member

@theoludwig theoludwig left a comment

Choose a reason for hiding this comment

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

From what I can see, this looks pretty good. 👍
Thank you very much for taking this task.
(I did a new a commit to fix some linting issues)

It even works with top-level await support:
2022-02-04_09-19

Few things before merging:

  • Add a new automated test inside client/testFixture to test standard@17.0.0-2 to avoid regression.
  • Investigate what happens if we for example use top-level await while using standard@16 (without ESLint v8 support hence not supporting top-level await), the extension should not crash.

@NemoStein
Copy link
Contributor Author

  • Add a new automated test inside client/testFixture to test standard@17.0.0-2 to avoid regression.

Not sure how to approach this one...
Adding await new Promise(resolve => resolve('')) to no-lint-error files would trigger an error when standard < 17 (as expected, since it doesn't support the syntax)
Maybe check module version and call different files, but that seems a bit smelly (and I don't really know if it's doable)

  • Investigate what happens if we for example use top-level await while using standard@16 (without ESLint v8 support hence not supporting top-level await), the extension should not crash.

I tested both standard@latest and standard@next (globally), both working as expected, including erroring on unsupported syntax.
The extension didn't crash in any case.

@divlo if you point me in the right direction, I'll be glad to update this PR with anything necessary to merge it.

@theoludwig
Copy link
Member

theoludwig commented Feb 4, 2022

Not sure how to approach this one...
Maybe check module version and call different files, but that seems a bit smelly (and I don't really know if it's doable)
@divlo if you point me in the right direction, I'll be glad to update this PR with anything necessary to merge it.

To be honest, I don't know either, I don't affect this code too often. As the original maintainer of this repo doesn't maintain it anymore, I decided to maintain it, but most of the code has been written by the original maintainer.


About the integration test part, yes indeed, that could be a little bit harder than I thought.
I didn't remember very well my own code I had written in October (in this PR: #297) 😂.
To do the testing we are currently installing globally the different engines thanks to this script: https://github.com/standard/vscode-standard/blob/master/scripts/e2e.sh
But if we want to test with 2 different versions of standard this technique no longer works.
So we could instead try to install "locally" the needed "engine" (standard, semistandard etc.) before running/launching each folder fixture (correspond to each supported engine) inside client/testFixture.


Whether we succeed or not to do that new integration test, it is definitely non-blocking for merging this PR.
We'll probably merge the PR this weekend, we don't need to wait too much and then do a new release 2.1.0 directly after this PR has been merged.

I tested both standard@latest and standard@next (globally), both working as expected, including erroring on unsupported syntax.
The extension didn't crash in any case.

Great, you already did a really good job, I also tested it myself, and indeed, everything is working fine. 👍 @NemoStein
I would rather have an automated test if we can because it would be a little bit safer and just nicer.
I will try to do something tomorrow, to see if we can add this integration test, if I can't figure it out, not a big deal, we'll merge.

Feel free to give ideas about how can we do integration testing for this VSCode extension.

@theoludwig theoludwig changed the title Add support for Standard 17 feat: add support for standard-engine@15 Feb 6, 2022
@theoludwig theoludwig merged commit b70aca7 into standard:master Feb 6, 2022
@welcome
Copy link

welcome bot commented Feb 6, 2022

🎉 Congrats on getting your first pull request landed!

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

Successfully merging this pull request may close these issues.

Add support for Standard 17
2 participants