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

Remove hardcoded file ignore string check #11

Merged
merged 1 commit into from
Aug 4, 2023

Conversation

yuxincs
Copy link
Contributor

@yuxincs yuxincs commented Jul 26, 2023

This PR removes the hardcoded nolint:nilaway docstring check for both files and function declarations since it is no longer necessary:

  • files: we have implemented configuration analyzer in Add a NilAway config analyzer for parsing and sharing configurations #9 which is able to take a list of strings to ignore the analysis of a file.

  • function declaration: it is mainly designed for two reasons:

    • performance: we were able to use it to manually skip the analysis of a particular function for performance reasons. However, we now have a timeout for analysis of any particular function, hence this is no longer required.
    • error suppression: most linter drivers, especially nogo, are now respecting nolint, so there is really no need to support it within the linter itself.

Depends on #10

@yuxincs yuxincs self-assigned this Jul 26, 2023
@shubhamugare
Copy link
Contributor

What do you mean by "most linter drivers, especially nogo, are now able to respect nolint itself"? There is an alternate way to suppress certain functions using nolint?

@yuxincs
Copy link
Contributor Author

yuxincs commented Jul 27, 2023

What do you mean by "most linter drivers, especially nogo, are now able to respect nolint itself"? There is an alternate way to suppress certain functions using nolint?

nogo was not respecting nolint at all until bazelbuild/rules_go#3561 was merged. Now that driver is able to parse the nolint comment, we do not really need to have that logic within NilAway anymore. Plus, the driver-side implementation is more general to handle suppression of multiple linter messages like //nolint:nilaway,errcheck.

Rephrased the PR decription 👍

@yuxincs yuxincs force-pushed the yuxincs/remove-hardcoded-file-ignore-string-check branch from b6391b7 to 92dced1 Compare July 28, 2023 14:59
@yuxincs yuxincs force-pushed the yuxincs/replace-is-test-environment branch from 1bb9133 to 717021a Compare July 28, 2023 14:59
@yuxincs yuxincs force-pushed the yuxincs/remove-hardcoded-file-ignore-string-check branch 2 times, most recently from e5deadf to 2d16cfe Compare July 31, 2023 17:20
@yuxincs yuxincs force-pushed the yuxincs/replace-is-test-environment branch from 64b66ac to 2a06f68 Compare July 31, 2023 17:20
@yuxincs yuxincs force-pushed the yuxincs/remove-hardcoded-file-ignore-string-check branch from 2d16cfe to c265f5b Compare July 31, 2023 18:35
@yuxincs yuxincs force-pushed the yuxincs/replace-is-test-environment branch 2 times, most recently from f665ba1 to 7456a3b Compare July 31, 2023 19:44
@yuxincs yuxincs force-pushed the yuxincs/remove-hardcoded-file-ignore-string-check branch from c265f5b to 6c21883 Compare July 31, 2023 19:44
@yuxincs yuxincs force-pushed the yuxincs/replace-is-test-environment branch from 7456a3b to e92f4de Compare July 31, 2023 19:51
@yuxincs yuxincs force-pushed the yuxincs/remove-hardcoded-file-ignore-string-check branch 2 times, most recently from b07ae5d to eb66fc4 Compare July 31, 2023 20:41
@yuxincs yuxincs force-pushed the yuxincs/replace-is-test-environment branch from e92f4de to 6990af9 Compare July 31, 2023 20:41
Copy link
Collaborator

@lazaroclapp lazaroclapp left a comment

Choose a reason for hiding this comment

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

LGTM!

@yuxincs yuxincs force-pushed the yuxincs/replace-is-test-environment branch from 6990af9 to d6e0736 Compare August 3, 2023 19:04
@yuxincs yuxincs force-pushed the yuxincs/remove-hardcoded-file-ignore-string-check branch from eb66fc4 to c725ec0 Compare August 3, 2023 19:04
@yuxincs yuxincs force-pushed the yuxincs/replace-is-test-environment branch from d6e0736 to fe2b512 Compare August 4, 2023 02:53
@yuxincs yuxincs force-pushed the yuxincs/remove-hardcoded-file-ignore-string-check branch 2 times, most recently from 1ce63c1 to 42256c8 Compare August 4, 2023 02:59
@yuxincs yuxincs force-pushed the yuxincs/replace-is-test-environment branch from fe2b512 to 4db2508 Compare August 4, 2023 03:00
@yuxincs yuxincs force-pushed the yuxincs/remove-hardcoded-file-ignore-string-check branch from 42256c8 to 6a44aa6 Compare August 4, 2023 03:00
Base automatically changed from yuxincs/replace-is-test-environment to main August 4, 2023 18:13
@yuxincs yuxincs force-pushed the yuxincs/remove-hardcoded-file-ignore-string-check branch from 6a44aa6 to d5a242f Compare August 4, 2023 18:15
@yuxincs yuxincs merged commit 1b2bb3f into main Aug 4, 2023
3 checks passed
@yuxincs yuxincs deleted the yuxincs/remove-hardcoded-file-ignore-string-check branch August 4, 2023 18:27
sonalmahajan15 pushed a commit that referenced this pull request Aug 5, 2023
This PR removes the hardcoded `nolint:nilaway` docstring check for both
files and function declarations since it is no longer necessary:

* files: we have implemented configuration analyzer in #9 which is able
to take a list of strings to ignore the analysis of a file.

* function declaration: it is mainly designed for two reasons:
* performance: we were able to use it to manually skip the analysis of a
particular function for performance reasons. However, we now have a
timeout for analysis of any particular function, hence this is no longer
required.
* error suppression: most linter drivers, especially nogo, are now
respecting `nolint`, so there is really no need to support it within the
linter itself.

Depends on #10
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.

3 participants