Skip to content

Conversation

@yoff
Copy link
Contributor

@yoff yoff commented Oct 6, 2020

This PR adds flow from the operands of IfExprNodes to the nodes themselves. It leans on the logic already implemented to exclude false positives in simple cases.

The dataflow step is simply added to essaFlowStep; we should perhaps rename that predicate...

Since I added new test cases to demonstrate that some false positives do not appear, I also formatted the test file with my new formatter of choice (black). This means that many line numbers are shifted, but the upside is that comments are closer to what they comment than to the definition above rather than the other way around. It also saves som lines overall.

Reading the commits in order might help, since all line shifting happens on the first commit.

yoff added 2 commits October 6, 2020 10:49
Also use better formatter
(better because comments are close to what they comment)
@yoff yoff requested a review from a team as a code owner October 6, 2020 08:57
@github-actions github-actions bot added the Python label Oct 6, 2020
# 6.2.3. Parenthesized forms
def test_parenthesized_form():
x = (SOURCE)
x = SOURCE
Copy link
Contributor

Choose a reason for hiding this comment

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

Did the formatter get a bit aggressive here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah, probably. I missed that, thanks!

Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

It would be super nice if you always put autoformatting commits in a separate commit, since it makes things a bit easier to scan. Now the title of 8f13d58 Python: More tests of conditonals lead me to believe that there would be new test-cases, but after looking through the diff of the test.py file 3 times, I haven't found any. Maybe just confusing commit message? (at least consider me confused)

Otherwise, I think we need to restore the parenthesis test. code-formatters are pretty nice, but sometimes it just doesn't do what we want :( besides this, change LGTM 👍

@yoff
Copy link
Contributor Author

yoff commented Oct 6, 2020

There are two new tests in there: 8f13d58#diff-601e823041b1218704b390c8387e5bb6R412-R426
I agree that they are near impossible to spot in that view though. Doing the autoformatting on its own is the right way, thanks for that suggestion. Would you like me to rebase to do that?

@RasmusWL
Copy link
Member

RasmusWL commented Oct 6, 2020

I agree that they are near impossible to spot in that view though. Doing the autoformatting on its own is the right way, thanks for that suggestion. Would you like me to rebase to do that?

Nah, lets just keep it in mind moving forwards 👍

@codeql-ci codeql-ci merged commit 5bc7e19 into github:main Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants