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

Prevent multiple messages for one violation #23

Merged
merged 6 commits into from
Jun 27, 2023

Conversation

yashincontrol
Copy link
Collaborator

@yashincontrol yashincontrol commented Jun 15, 2023

Fixes #22. The problem is that the no-internal rule was generating duplicate error messages. This was because the rule was not correctly handling the traversal of declarations and their parents, leading to redundant checks and violation messages. Fixed the function calls and added a visitedDeclarations set to avoid duplicate processing of declarations.

ben-polinsky
ben-polinsky previously approved these changes Jun 16, 2023
@yashincontrol yashincontrol marked this pull request as draft June 16, 2023 16:13
@ben-polinsky ben-polinsky marked this pull request as ready for review June 23, 2023 20:26
@ben-polinsky ben-polinsky marked this pull request as draft June 23, 2023 20:26
Copy link
Collaborator

@ben-polinsky ben-polinsky left a comment

Choose a reason for hiding this comment

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

Looks better.
In general, the tests are a bit confusing due to the @bentley/@iTwin scope option/requirement. Could you add a comment next to the import(s) clarifying why the code is valid or invalid? Something like "not a bentley/itwin scope" or "is a local import"

@yashincontrol
Copy link
Collaborator Author

Looks better. In general, the tests are a bit confusing due to the @bentley/@iTwin scope option/requirement. Could you add a comment next to the import(s) clarifying why the code is valid or invalid? Something like "not a bentley/itwin scope" or "is a local import"

Yes, will add that

@yashincontrol yashincontrol marked this pull request as ready for review June 27, 2023 14:30
Copy link
Contributor

@MichaelBelousov MichaelBelousov left a comment

Choose a reason for hiding this comment

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

lgtm, minor comments mostly formatting nits which can be resolved at your leisure

dist/rules/no-internal.js Outdated Show resolved Hide resolved
tests/no-internal.js Outdated Show resolved Hide resolved
tests/no-internal.js Outdated Show resolved Hide resolved
tests/no-internal.js Outdated Show resolved Hide resolved
tests/no-internal.js Outdated Show resolved Hide resolved
tests/no-internal.js Outdated Show resolved Hide resolved
@MichaelBelousov
Copy link
Contributor

Looks better. In general, the tests are a bit confusing due to the @bentley/@iTwin scope option/requirement. Could you add a comment next to the import(s) clarifying why the code is valid or invalid? Something like "not a bentley/itwin scope" or "is a local import"

Great idea, does make them much more readable.

@yashincontrol yashincontrol merged commit 73729f5 into main Jun 27, 2023
@yashincontrol yashincontrol deleted the yashasvi/fix-no-internal branch June 27, 2023 18:02
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.

Multiple messages for one violation of no-internal
3 participants