Skip to content

SONARPY-432 Support symbol highlighting #393

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 9, 2019
Merged

SONARPY-432 Support symbol highlighting #393

merged 3 commits into from
Oct 9, 2019

Conversation

guillaume-dequenne
Copy link
Contributor

No description provided.

@guillaume-dequenne guillaume-dequenne force-pushed the support_symbol_highlighting branch 5 times, most recently from 47dd708 to e914fe8 Compare October 8, 2019 15:02
benzonico
benzonico requested changes Oct 9, 2019
View reviewed changes
Copy link
Contributor

@benzonico benzonico left a comment

Choose a reason for hiding this comment

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

Quite some things in there : most interestingly, simpler code and stronger testing would have prevented to have a bug ;)

python-squid/src/main/java/org/sonar/python/api/tree/FileInput.java Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Name is dodgy : Those are global variables, neither module variables or global fields. I think we should name this globalVariables.

python-squid/src/test/java/org/sonar/python/semantic/SymbolTableBuilderTreeTest.java Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

assertThat(moduleSymbols).extracting(name).containsExactly("global_x", "global_var");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't this assert on the order as well? which can't be guaranteed for a Set?

python-squid/src/main/java/org/sonar/python/semantic/SymbolTableBuilder.java Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

not related to your PR : I am just wondering if we could just avoid 3 iterations over values and have only one with a plain old for loop. If you're up to it, I think it is worth a commit of refactoring at the end of your PR.

sonar-python-plugin/src/test/java/org/sonar/plugins/python/PythonSquidSensorTest.java Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need to collect to assert on a stream. And moreover, the containsOnly assertion will check it is not Empty, so no need for the isNotEmpty call.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please note that you only tested START of references and you most probably have a bug of end of references.

sonar-python-plugin/src/test/java/org/sonar/plugins/python/SymbolVisitorTest.java Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the dir variable ? I think you can inline it and concatenate it to the file name.

sonar-python-plugin/src/test/java/org/sonar/plugins/python/SymbolVisitorTest.java Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could remove componentKey as parameter of this method and use the field directly.

sonar-python-plugin/src/main/java/org/sonar/plugins/python/SymbolVisitor.java Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can store firstUsage.tree() in a dedicated var instead of calling it 4 times ?

sonar-python-plugin/src/main/java/org/sonar/plugins/python/SymbolVisitor.java Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can store usage.tree() in a dedicated var instead of calling it 4 times ? which makes me realize that we might have a copy/paste bug here : there is a firstUsage.tree() call !!! Why is this not caught by unit test ? (seems that ending of reference is not tested)

@guillaume-dequenne guillaume-dequenne force-pushed the support_symbol_highlighting branch 2 times, most recently from 623eaa7 to 8fc6a37 Compare October 9, 2019 11:46
benzonico
benzonico requested changes Oct 9, 2019
View reviewed changes
python-squid/src/main/java/org/sonar/python/tree/FileInputImpl.java Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

This ought to be rename to addGlobalVariable (and the param name as well)

sonar-python-plugin/src/test/java/org/sonar/plugins/python/SymbolVisitorTest.java Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably don't need the condition.

sonar-python-plugin/src/test/java/org/sonar/plugins/python/PythonSquidSensorTest.java Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably don't need the condition

@guillaume-dequenne guillaume-dequenne force-pushed the support_symbol_highlighting branch from 8fc6a37 to 53a4425 Compare October 9, 2019 12:34
@guillaume-dequenne guillaume-dequenne force-pushed the support_symbol_highlighting branch from 53a4425 to 89f2c1a Compare October 9, 2019 12:36
benzonico
benzonico approved these changes Oct 9, 2019
View reviewed changes
Copy link
Contributor

@benzonico benzonico left a comment

Choose a reason for hiding this comment

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

see last minor details. Otherwise LGTM

sonar-python-plugin/src/test/java/org/sonar/plugins/python/SymbolVisitorTest.java Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation.

sonar-python-plugin/src/main/java/org/sonar/plugins/python/PythonScanner.java Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny detail but I personally think it would be more readable with :

new SymbolVisitor(context.newSymbolTable().onFile(inputFile)).visitFileInput(visitorContext.rootTree());

feel free to discard the suggestion.

@guillaume-dequenne guillaume-dequenne force-pushed the support_symbol_highlighting branch from 89f2c1a to 6e1d2c1 Compare October 9, 2019 13:17
@guillaume-dequenne guillaume-dequenne merged commit b1d4bd5 into master Oct 9, 2019
@guillaume-dequenne guillaume-dequenne deleted the support_symbol_highlighting branch October 9, 2019 13:28
hashicorp-vault-sonar-prod bot pushed a commit that referenced this pull request Jul 24, 2025
@joke1196
SONARPY-2441: Fix NPE when qualified expression is used as keyword ar…
3b1be31 
…gument (#393)

GitOrigin-RevId: 329a636f233c46a099cfa1b70c5c24c08ac2a37e
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