Skip to content

Migrate TrailingCommentCheck to Strongly typed AST#357

Merged
benzonico merged 23 commits into
masterfrom
migrate_TrailingCommentCheck
Sep 30, 2019
Merged

Migrate TrailingCommentCheck to Strongly typed AST#357
benzonico merged 23 commits into
masterfrom
migrate_TrailingCommentCheck

Conversation

@guillaume-dequenne
Copy link
Copy Markdown
Contributor

@guillaume-dequenne guillaume-dequenne commented Sep 24, 2019

No description provided.

@guillaume-dequenne guillaume-dequenne force-pushed the migrate_TrailingCommentCheck branch 2 times, most recently from 8fac083 to 55e09ed Compare September 24, 2019 09:19
benzonico
benzonico requested changes Sep 24, 2019
View reviewed changes
Comment thread python-checks/src/main/java/org/sonar/python/checks/TrailingCommentCheck.java Outdated
Copy link
Copy Markdown
Contributor

@benzonico benzonico Sep 24, 2019

Choose a reason for hiding this comment

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

Please inline it no need to have a public constant here. Thanks.

Comment thread python-checks/src/main/java/org/sonar/python/checks/TrailingCommentCheck.java Outdated
Copy link
Copy Markdown
Contributor

@benzonico benzonico Sep 24, 2019

Choose a reason for hiding this comment

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

This import is supsicious : we should not be requiring implementation.

Comment thread python-checks/src/main/java/org/sonar/python/checks/TrailingCommentCheck.java Outdated
Copy link
Copy Markdown
Contributor

@benzonico benzonico Sep 24, 2019

Choose a reason for hiding this comment

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

Checks should rely on PyToken rather than token.

Comment thread python-checks/src/main/java/org/sonar/python/checks/TrailingCommentCheck.java Outdated
Copy link
Copy Markdown
Contributor

@benzonico benzonico Sep 24, 2019

Choose a reason for hiding this comment

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

Why is an ignore list required ? if we are visiting the same token multiple times we probably have an issue into the visit of tokens

Comment thread python-squid/src/test/java/org/sonar/python/PythonSubscriptionCheckTest.java Outdated
Copy link
Copy Markdown
Contributor

@benzonico benzonico Sep 24, 2019

Choose a reason for hiding this comment

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

Same remark, SSLR token should not leak from PyToken api.
IMO we should mark the token() method in PyToken as deprecated : this method is there for issue location but should not be used in checks to not leak SSLR API in checks.

Comment thread python-squid/src/test/java/org/sonar/python/PythonSubscriptionCheckTest.java Outdated
Copy link
Copy Markdown
Contributor

@benzonico benzonico Sep 24, 2019

Choose a reason for hiding this comment

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

Same remark : why an ignore list ?

@guillaume-dequenne guillaume-dequenne force-pushed the migrate_TrailingCommentCheck branch 12 times, most recently from c19476b to f14d814 Compare September 27, 2019 07:51
@benzonico benzonico force-pushed the migrate_TrailingCommentCheck branch 2 times, most recently from 68e1610 to 7d4f316 Compare September 27, 2019 10:09
@guillaume-dequenne guillaume-dequenne force-pushed the migrate_TrailingCommentCheck branch from ac57ca1 to 762f6e6 Compare September 27, 2019 13:11
@benzonico benzonico force-pushed the migrate_TrailingCommentCheck branch from b248d79 to c7c80e2 Compare September 30, 2019 07:08
benzonico
benzonico reviewed Sep 30, 2019
View reviewed changes
Comment thread python-squid/src/test/java/org/sonar/python/tree/PythonTreeMakerTest.java Outdated
Copy link
Copy Markdown
Contributor

@benzonico benzonico Sep 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think this test can be more robust by checking that this token should be equal to "firstToken" ( and same for "lastToken" )

@guillaume-dequenne guillaume-dequenne force-pushed the migrate_TrailingCommentCheck branch from 37e2526 to b363089 Compare September 30, 2019 12:54
@guillaume-dequenne guillaume-dequenne force-pushed the migrate_TrailingCommentCheck branch from 68bd01f to ead763e Compare September 30, 2019 14:00
@guillaume-dequenne guillaume-dequenne force-pushed the migrate_TrailingCommentCheck branch from ead763e to 97b632e Compare September 30, 2019 14:14
@benzonico benzonico merged commit 6dd949a into master Sep 30, 2019
@benzonico benzonico deleted the migrate_TrailingCommentCheck branch September 30, 2019 15:26
hashicorp-vault-sonar-prod Bot pushed a commit that referenced this pull request Jul 4, 2025
@ghislainpiot
SONARPY-3145 Increase coverage of Error branch in the CompletableFutu…
1504979 
…re exception handling (#357)

GitOrigin-RevId: 6feb846c827dcf179a3bf627cb8f8063d122625b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benzonico benzonico benzonico approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@guillaume-dequenne @benzonico