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

Improve float to-to-top behavior with existing imports #1502

Closed
timothycrosley opened this issue Sep 27, 2020 · 4 comments · Fixed by #1503
Closed

Improve float to-to-top behavior with existing imports #1502

timothycrosley opened this issue Sep 27, 2020 · 4 comments · Fixed by #1503
Labels
enhancement New feature or request
Milestone

Comments

@timothycrosley
Copy link
Member

It would be nice for the float to top setting to respect at least one single new line after the top of file comment if it already exists with an import statement. This would make it more usable for projects that want to enforce imports done at the top of the file, instead of just using it for its initial intended usage of being a short cut to push imports to the top. See comment from @char101 here: #1499

@char101
Copy link

char101 commented Sep 27, 2020

In my limited testing, my previous patch (this line and not line.strip() != "") do exhibit that behaviour (keeps existing new line, compress multiple lines into a single blank line, does not add a blank line if there is none initially).

#1499 (comment)

@timothycrosley
Copy link
Member Author

@char101

I believe it's likely your solution worked in your testing only because it has a logic error that causes it to skip all code chunks and default only to the placement of the first import. So, within your own code snippets, which already had the imports placed it would work. As an example with that change, this example would be unchanged despite float to top:

"""my source
comment
"""


def my_function():
   pass


import  os

Your change added and not line.strip() != "" which is the same as saying the stripped line must be == to "", combined with the check that the line is not empty above, it means that entire code block is essentially always skipped.

@timothycrosley timothycrosley added this to the 5.6.0 milestone Sep 27, 2020
timothycrosley added a commit that referenced this issue Sep 27, 2020
…ing import section present at top-of-file.
timothycrosley added a commit that referenced this issue Sep 27, 2020
…hen-existing-import-section-present

Resolved #1502: Improved float-to-top behavior when there is an exist…
@timothycrosley
Copy link
Member Author

This is now fixed in develop and will be released in the 5.6.0 release of isort in early October.

Note that there is one intentional caveat that if there are no imports pre-existing at the top of the file isort puts them immediately underneath the comment.

For example:

"""My comment"""


def my_function():
   pass


import os

Will intentionally turn into:

"""My comment"""
import os


def my_function():
   pass

This is because, if you don't have any imports at the top isort can't know how many lines you want and it has to pick some value for that. As shown in this example, it is common for 2 lines to be between functions, but if isort chose to keep those two lines, the behaviour would likely not be what is intended:

"""My comment"""


import os


def my_function():
   pass

@timothycrosley
Copy link
Member Author

Update: this has just been released to PyPI in 5.6.0 release of isort: https://pycqa.github.io/isort/CHANGELOG/#560-october-7-2020

Thanks!

~Timothy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants