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

Demonstrate fixme comments that are not being respected #413

Conversation

samueljsb
Copy link
Contributor

Summary

Test Plan

Failing tests have been added to demonstrate (most of) the bugs reported in #405. I have not been able to add one for the function definition problem because there are cases in this test that need to define functions without errors, so I can't just report functionDef.

@facebook-github-bot
Copy link

Hi @samueljsb!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@samueljsb samueljsb changed the title 405 lint fixme comments not respected Demonstrate fixme comments that are not being respected Dec 1, 2023
@samueljsb samueljsb force-pushed the 405-lint-fixme-comments-not-respected branch from ea50956 to a3099fd Compare December 1, 2023 12:16
@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 1, 2023
@amyreese amyreese force-pushed the 405-lint-fixme-comments-not-respected branch from a3099fd to 3bb7dd2 Compare April 5, 2024 22:58
Adds logic to `LintRule.node_comments()` looking for directives in more
locations relative to the current node:

- trailing inline comments after commas
- preceding and trailing comments inside brackets
- preceding comments before/after decorators for classes and functions

In addition, to better support suppressions for decorators themselves,
the logic to break upward searching until reaching a `leading_lines`
node has been modified to exclude `Decorator` nodes which have their own
leading lines that aren't used for the first decorator on a class.

Based on code examples reported in Instagram#413
@amyreese amyreese force-pushed the 405-lint-fixme-comments-not-respected branch from 3bb7dd2 to adbd956 Compare April 20, 2024 01:08
Add a failing test to demonstrate decorator comment not respected

Add failing test to demonstrate comprehension comments not respected

Add failing test to demonstrate list element comments not respected

More test cases, label and split existing cases

Co-authored-by: Amethyst Reese <amethyst@n7.gg>
@amyreese
Copy link
Member

Thank you so much for these examples! I've got a fix in #441, and squashed this PR with some additional test cases into #442 (retaining your authorship).

@amyreese amyreese closed this Apr 20, 2024
amyreese added a commit that referenced this pull request Apr 20, 2024
Adds logic to `LintRule.node_comments()` looking for directives in more
locations relative to the current node:

- trailing inline comments after commas
- preceding and trailing comments inside brackets
- preceding comments before/after decorators for classes and functions

In addition, to better support suppressions for decorators themselves,
the logic to break upward searching until reaching a `leading_lines`
node has been modified to exclude `Decorator` nodes which have their own
leading lines that aren't used for the first decorator on a class.

Based on code examples reported in #413

ghstack-source-id: 4907870d9ecde15fd1110b3d276bc7f95569c513
Pull Request resolved: #441
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants