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 no-duplicate-store-ids #15

Merged
merged 3 commits into from
Apr 13, 2024
Merged

feat: add no-duplicate-store-ids #15

merged 3 commits into from
Apr 13, 2024

Conversation

9romise
Copy link
Collaborator

@9romise 9romise commented Apr 7, 2024

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What did you change?

Add no-duplicate-store-ids, closes #5

Why did you change it?

This is a new rule

Other information

I'm intersted on it and willing to contribute. Here is a commit with my rough idea. I'm not sure if there's a better one. If you have any suggestions, I'm looking forward to hearing them.

Copy link
Owner

@lisilinhart lisilinhart left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, I have some minor feedback, then we can merge it 🙂

src/index.ts Outdated Show resolved Hide resolved
src/rules/no-duplicate-store-ids.ts Show resolved Hide resolved
src/rules/no-duplicate-store-ids.ts Show resolved Hide resolved
},
schema: [],
messages: {
duplicatedStoreIds: 'No duplicated store ids allowed.'
Copy link
Owner

Choose a reason for hiding this comment

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

Let's add more context here:

Suggested change
duplicatedStoreIds: 'No duplicated store ids allowed.'
duplicatedStoreIds: 'No duplicated store ids allowed: {{storeId}}'


if (usingStoreIds.has(value)) {
context.report({
node,
Copy link
Owner

Choose a reason for hiding this comment

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

We can report on the actual literal node to make it more clear.

Suggested change
node,
node: storeId,

if (usingStoreIds.has(value)) {
context.report({
node,
messageId: 'duplicatedStoreIds'
Copy link
Owner

Choose a reason for hiding this comment

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

And add the context here:

Suggested change
messageId: 'duplicatedStoreIds'
messageId: 'duplicatedStoreIds',
data: {
storeId: storeId.value
}

Copy link
Owner

@lisilinhart lisilinhart left a comment

Choose a reason for hiding this comment

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

Hi @9romise, I tested your change here locally and you're right that it can only report across the files with the Set outside the eslint context. I'm not 100% if this might cause side effects, but it seems to work, so we can try adding it for now. I think we can just add a little bit more context and report closer to where the actual node is:

image

@9romise
Copy link
Collaborator Author

9romise commented Apr 13, 2024

Hi @lisilinhart, I think your suggestion is correct, I've completed these changes.

@lisilinhart lisilinhart merged commit 5b174b7 into lisilinhart:main Apr 13, 2024
6 checks passed
Copy link

🎉 This PR is included in version 0.1.13 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

Rule: Unique store ids
2 participants