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

Ensure import lines with ';' are properly split and auto corrected #177

Conversation

superDross
Copy link
Contributor

Resolves #176

Does so by import lines with semicolon separators are split into new lines before being processed.
e.g. import pdb; pdb.set_trace() would become import pdb\npdb.set_trace().

Checklist

  • [ x] Add test cases to all the changes you introduce
  • [ x] Update the documentation for the changes

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1714771961

  • 12 of 12 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 96.104%

Totals Coverage Status
Change from base Build 1712054575: 0.1%
Covered Lines: 296
Relevant Lines: 308

💛 - Coveralls

@lyz-code
Copy link
Owner

Hi @superDross thank you again for opening the issue and the PR. Your implementation works perfectly, but I wonder if we could avoid another iteration over the code lines.

The import ... ; ... structure is not often used in python scripts despite the use of the breaking point you mentioned in #176. The way I define breakpoints is with __import__("pdb").set_trace() # XXX BREAKPOINT which doesn't get changed by autoimport.

My concerns are that with the current solution we need to iterate over all the lines of the code to discern if there are ; in import statements, which slows down the run on autoimport for all the source codes for a corner case. I know is what we do in every filter, but the less we have, the better.

I agree that your breaking point snippet is the most popular one, so it make sense that we handle it. Could you please take a look if you can implement the logic avoiding yet another iteration? Maybe tweaking _move_imports_to_top by sending the left operand of the ; to the self.imports.append and changing the line content to the left of the operand.

@superDross
Copy link
Contributor Author

I agree this solution is supoptimal, in fact I am not terribly proud of the solution I proposed here. I simply wanted a quick and dirty solution as this bug was really irritating me when using it locally and thought I should share it.

I will work on creating a better solution at some point. I don't have an ETA for an update to the PR though, just depends when I get some free time.

Thanks for your input.

@superDross superDross force-pushed the fix/imports-with-semicolons-correctly-moved branch from 4aa944e to ba4d3e4 Compare January 23, 2022 20:35
@superDross superDross force-pushed the fix/imports-with-semicolons-correctly-moved branch from ba4d3e4 to 07e8af0 Compare January 23, 2022 20:40
@superDross
Copy link
Contributor Author

I have updated the PR with a more optimal solution.

It seems pushing this branch up did not trigger the pipeline.

@lyz-code
Copy link
Owner

Now the solution looks perfect.

I'm not sure why the CI didn't trigger, it will on the merge commit,

Thanks for your contribution :)

@lyz-code lyz-code merged commit 64706ec into lyz-code:master Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Import lines with a semicolon separator are improperly formatted
3 participants