Skip to content

Add new rule no-invalid-dependent-keys #709

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

Merged
merged 3 commits into from
Mar 20, 2020
Merged

Conversation

TheMBTH
Copy link

@TheMBTH TheMBTH commented Mar 15, 2020

This rule aims to avoid mismatched open and closed braces.

As discussed in the comments of this issue #105, I added a rule to check that curly braces are balanced within a computed properties' parameters

Fixes #708.

@TheMBTH TheMBTH force-pushed the ISSUE-708 branch 7 times, most recently from bf55402 to ab8f56b Compare March 15, 2020 16:06
@TheMBTH
Copy link
Author

TheMBTH commented Mar 15, 2020

Sorry about the multiple commits & pushes. This is the second time I push and PR on Github 😕 Let me know if I need to change something 😃 .
I also missed something obvious about recommended rules because I created the tests before running generator-eslint.

I struggled a bit to have the CI pass so I will another issue to improve documentation for "newbies" like me 😛

Copy link
Member

@bmish bmish left a comment

Choose a reason for hiding this comment

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

Excited to finally see a rule like this!

@bmish bmish changed the title ISSUE #708 - Add rule "balanced-braces" to check matching curly braces Add new rule balanced-braces Mar 15, 2020
@TheMBTH TheMBTH force-pushed the ISSUE-708 branch 3 times, most recently from 835aa39 to db8bbd1 Compare March 16, 2020 20:52
@bmish
Copy link
Member

bmish commented Mar 16, 2020

@TheMBTH can you remove the unrelated commits?

@bmish bmish changed the title Add new rule balanced-braces Add new rule no-invalid-dependent-keys Mar 17, 2020
… that dependent keys used for computed properties to be valid.
@TheMBTH
Copy link
Author

TheMBTH commented Mar 17, 2020

I rebased and removed the unrelated commits. I rarely rebase so I am not sure about the result.
I wanted to have README updated so I pulled master. How should we keep applying changes from master while we are still working on a PR?
thx 🙏

Copy link
Member

@bmish bmish left a comment

Choose a reason for hiding this comment

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

Almost done!

You should rebase on the latest master frequently, this guide might help: https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request

context.report({
node,
message: ERROR_MESSAGE,
loc: {
Copy link
Member

Choose a reason for hiding this comment

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

loc should be removed. Instead, the node that is reported should be string literal node with the violation.

fix: report violations on string literals in `no-invalid-dependent-keys` rule
test: add more valid test cases for `no-invalid-dependent-keys` rule
Copy link
Member

@bmish bmish left a comment

Choose a reason for hiding this comment

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

Thanks! (I added commits to fix a few things for you.)

@bmish bmish merged commit 78d8ec4 into ember-cli:master Mar 20, 2020
@TheMBTH
Copy link
Author

TheMBTH commented Mar 21, 2020

Hi @bmish .... Sorry for the delay. I didn't know there was a release coming and couldn't find the time.
Thanks again for the help. I'll make sure to check your updates.

@TheMBTH
Copy link
Author

TheMBTH commented Mar 21, 2020

A few questions regarding your changes:
1 - My tests passed with 'nodeType' because and error needs a type. So the type was actually not checked. It's the message that has a nodeType. Am I correct?
1 - The tests still pass without these options. What was the pupose of adding them?
parserOptions: { ecmaVersion: 6, sourceType: 'module', ecmaFeatures: { legacyDecorators: true }, },

Have a great week-end 😸

Cheers

@bmish
Copy link
Member

bmish commented Mar 21, 2020

Thanks for contributing @TheMBTH!

  1. Each error object in invalid test cases needs to specify the type of node that is reported as the violation. I changed your rule to report violations on specific string literal nodes and then updated the corresponding tests.
  2. If you look at the latest master, I added a test to ensure your rule works with the @computed decorator, which needs additional parserOptions.

@TheMBTH
Copy link
Author

TheMBTH commented Mar 21, 2020

Okay thx.
1- understood
2- I saw the test about @computed but when I removed the parserOptions to see check how it works, the tests still passed by passing parser: require.resolve('babel-eslint') only.
I'll look at it on my end to understand.

@bmish
Copy link
Member

bmish commented Mar 21, 2020

I see, if any of the new parserOptions are unnecessary, feel free to remove them!

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

Successfully merging this pull request may close these issues.

Curly braces used in computed properties has to be balanced.
2 participants