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

fix(eslint-plugin): [no-output-on-prefix] correct false positives #525

Merged
merged 2 commits into from Jun 21, 2021

Conversation

rafaelss95
Copy link
Member

@rafaelss95 rafaelss95 commented May 31, 2021

This PR basically fixes the same problems related in #523, but for no-output-on-prefix.

@JamesHenry while doing this I just noticed that we aren't checking the outputs metadata property. Example:

@Component({
  outputs: ['onChange'],
})
class Test {}

Should we support it here and in other *-output-* rules? If so, I can work on this on another PRs.

@rafaelss95 rafaelss95 force-pushed the fix/no-output-on-prefix branch 2 times, most recently from d61322e to c568b24 Compare May 31, 2021 15:43
@nx-cloud
Copy link

nx-cloud bot commented May 31, 2021

Nx Cloud Report

CI ran the following commands for commit 00bcb43. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=build --all --parallel
#000000 nx run-many --target=check-configs --all --parallel
#000000 nx run-many --target=test --all --parallel
#000000 nx run-many --target=typecheck --all --parallel

Sent with 💌 from NxCloud.

@rafaelss95 rafaelss95 force-pushed the fix/no-output-on-prefix branch 6 times, most recently from 5f9a3a4 to a3f1578 Compare May 31, 2021 21:22
@rafaelss95
Copy link
Member Author

Idk why the snapshot test is failing as I didn't touch the error message.

@JamesHenry
Copy link
Member

@rafaelss95 I made some major changes to integration tests to allow them to be run in parallel. Those changes combined with Nx Cloud Distributed CI has cut the total time for CI in half which is pretty amazing.

In light of these changes, it might be worth seeing if you can finally now run the integration tests on your local machine:

You can try the following to run them all serially (will take some time but hopefully it will work for you):

yarn integration-tests

You can just add -u at the end to run jest in update mode and update snapshots whenever required. I also kept the alias for this:

yarn update-integration-tests

@JamesHenry
Copy link
Member

And with regards to your comment:

Idk why the snapshot test is failing as I didn't touch the error message.

It's because you have changed the location of the error message, it is a legitimate failure:

image

@JamesHenry
Copy link
Member

JamesHenry commented Jun 20, 2021

while doing this I just noticed that we aren't checking the outputs metadata property. Example:

Hmm yeah interesting... Can you think of any reason why this wasn't done in Codelyzer this whole time? I wonder if it was intentional but I'm not sure why that would be the case

@codecov
Copy link

codecov bot commented Jun 20, 2021

Codecov Report

Merging #525 (00bcb43) into master (de37ee4) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #525   +/-   ##
=======================================
  Coverage   85.41%   85.41%           
=======================================
  Files          88       88           
  Lines        2358     2358           
  Branches      409      409           
=======================================
  Hits         2014     2014           
  Misses        199      199           
  Partials      145      145           
Impacted Files Coverage Δ
...ges/eslint-plugin/src/rules/no-output-on-prefix.ts 100.00% <100.00%> (ø)

@rafaelss95
Copy link
Member Author

@rafaelss95 I made some major changes to integration tests to allow them to be run in parallel. Those changes combined with Nx Cloud Distributed CI has cut the total time for CI in half which is pretty amazing.

In light of these changes, it might be worth seeing if you can finally now run the integration tests on your local machine:

You can try the following to run them all serially (will take some time but hopefully it will work for you):

yarn integration-tests

You can just add -u at the end to run jest in update mode and update snapshots whenever required. I also kept the alias for this:

yarn update-integration-tests

Impressive! That's really faster and I also noticed that my node version was 15, which is greather than the maximum suported 14. Everything should be fine now, thanks!

And with regards to your comment:

Idk why the snapshot test is failing as I didn't touch the error message.

It's because you have changed the location of the error message, it is a legitimate failure:

image

Danm, I didn't noticed that the location was different 😞

while doing this I just noticed that we aren't checking the outputs metadata property. Example:

Hmm yeah interesting... Can you think of any reason why this wasn't done in Codelyzer this whole time? I wonder if it was intentional but I'm not sure why that would be the case

I imagine that's the case of just something that was never requested because it's less used than traditional decorators, but 🤷‍♂️

@JamesHenry
Copy link
Member

Cool, fair enough, the yeah I think we should address the outputs: [] point too.

I strongly recommend you install https://volta.sh - it's absolutely flawless and node, npm and yarn version management.

All you need to do is CD into a project and it will magically switch your versions of those binaries to the right ones for that project. You can see the pinned versions we have in package.json (that's what it uses as its source of truth for a project, otherwise it defaults to LTS).

We also use volta in CI so that it never gets out of sync with local development

@JamesHenry JamesHenry merged commit 3a66274 into angular-eslint:master Jun 21, 2021
@rafaelss95 rafaelss95 deleted the fix/no-output-on-prefix branch June 22, 2021 05:46
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

2 participants