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

Fix and extra tests for literals #115

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

nielsdebruin
Copy link
Contributor

@nielsdebruin nielsdebruin commented Jan 9, 2025

What's changed?

The TabsAndIndentsVisitor cannot correctly handle literals that are not assigned to anything. This PR will fix this behavior and adds extra tests.

What's your motivation?

Anything in particular you'd like reviewers to focus on?

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

Any additional context

Checklist

  • I've added unit tests to cover both positive and negative cases
  • I've read and applied the recipe conventions and best practices
  • I've used the IntelliJ IDEA auto-formatter on affected files

@nielsdebruin nielsdebruin marked this pull request as ready for review January 10, 2025 09:36
@nielsdebruin nielsdebruin changed the title Fix multiline string literals Extra test for literals Jan 10, 2025
@nielsdebruin nielsdebruin changed the title Extra test for literals Extra tests for literals Jan 10, 2025
@nielsdebruin nielsdebruin changed the title Extra tests for literals Fix and extra tests for literals Jan 14, 2025
@knutwannheden
Copy link
Contributor

@nielsdebruin On my computer the test_comment_alignment_if_and_return test case fails with this PR. Can you try to confirm?

@nielsdebruin
Copy link
Contributor Author

@nielsdebruin On my computer the test_comment_alignment_if_and_return test case fails with this PR. Can you try to confirm?

It does, but I believe this problem is also present in main.

@knutwannheden
Copy link
Contributor

Yes, I think so. But with this PR the indent of the return statement is wrong, which I believe isn't the case on main.

@nielsdebruin
Copy link
Contributor Author

@knutwannheden I ran the test, I see the indentation, but there is not difference between my main, and this branch.

main:

('\n'
 'def my_function(a, b):\n'
 '    if a > b:\n'
 '        # cool\n'
 '        a = b + 1\n'
 '    # cool\n'
 '  return None\n')

This branch:

('\n'
 'def my_function(a, b):\n'
 '    if a > b:\n'
 '        # cool\n'
 '        a = b + 1\n'
 '    # cool\n'
 '  return None\n')

@knutwannheden
Copy link
Contributor

I thought that was working at some point. This definitely needs to be fixed so that we don't produce code that doesn't compile.

@knutwannheden
Copy link
Contributor

I suggest we try to fix that first, independently of this PR, and then we can see how we can merge this PR afterwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants