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

CLN: use absolute imports throughout the codebase #1682

Merged
merged 3 commits into from
Feb 13, 2023

Conversation

idanmiara
Copy link
Contributor

@idanmiara idanmiara commented Dec 20, 2022

Related to #1711
Replace relative imports (i.e. from . import xyz) with absolute imports (from shapely import xyz) and run isort as recommended by PEP8 (https://peps.python.org/pep-0008/#imports): Absolute imports are recommended, as they are usually more readable and tend to be better behaved.
This helps much with debugging.

@idanmiara
Copy link
Contributor Author

idanmiara commented Dec 21, 2022

Hi,
This PR ready for review.
I've rebased and cleaned commit history.
Thanks!

@coveralls
Copy link

coveralls commented Dec 21, 2022

Pull Request Test Coverage Report for Build 4161382935

  • 71 of 72 (98.61%) changed or added relevant lines in 17 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 87.085%

Changes Missing Coverage Covered Lines Changed/Added Lines %
shapely/geometry/conftest.py 0 1 0.0%
Totals Coverage Status
Change from base Build 4161378179: 0.3%
Covered Lines: 2387
Relevant Lines: 2741

💛 - Coveralls

@idanmiara idanmiara changed the title PEP8: replace relative imports (i.e. from . import xyz) with absolute imports (from shapely import xyz) BUG:PEP8: replace relative imports (i.e. from . import xyz) with absolute imports (from shapely import xyz) Jan 2, 2023
@idanmiara
Copy link
Contributor Author

@jorisvandenbossche please review

@idanmiara
Copy link
Contributor Author

@jorisvandenbossche can his be included in 2.0.1 ?

@jorisvandenbossche
Copy link
Member

Personally, I have a (slight, but not strong) preference for keeping the relative imports, because I like using those (there are also several other projects that do that as well, like numpy and scikit-learn, although those projects might fit more in the "complex package layouts" bucket compared to shapely)

There is certainly some inconsistency right now internally, because the usage of this has come from merging the pygeos code, while what remained from the original shapely code uses absolute imports.
So we should certainly choose one way or the other.

cc @caspervdw since you started using this originally in pygeos

@idanmiara
Copy link
Contributor Author

idanmiara commented Jan 7, 2023

@jorisvandenbossche Thanks for your comments.

Sorry, I'm not sure I follow what's the advantage of using relative imports (You noted that some other popular projects use them, but why is it an advantage? many other popular projects use absolute imports exclusively).

The disadvantages that I know of, for using relative imports are:

  1. It's a bad practice (https://peps.python.org/pep-0008/#imports).
  2. You can't copy and paste some code from a module and test it without modifying the imports.
  3. BUG/Regression: Running doctests fail because of relative imports #1711

@idanmiara
Copy link
Contributor Author

Hi @jorisvandenbossche any further thoughts about this PR and with respect to #1711 ?

@idanmiara
Copy link
Contributor Author

@brendan-ward @mwtoews @caspervdw any thoughts about this?

@brendan-ward
Copy link
Collaborator

Have not reviewed the PR (will try to do so in the coming days), but I agree with the direction; I stumbled with the relative imports in pygeos several times while trying to test code there, and the precedent for absolute imports was already set in shapely.

Copy link
Collaborator

@brendan-ward brendan-ward left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @idanmiara ! Only spotted a couple of issues (not sure why isort is putting unittest into wrong position), but otherwise this looks good to me.

It brings a nice consistency to the imports, since the precedent was already established in shapely < 2.0

shapely/io.py Outdated Show resolved Hide resolved
tests/test_affinity.py Outdated Show resolved Hide resolved
@idanmiara
Copy link
Contributor Author

Thanks for working on this @idanmiara ! Only spotted a couple of issues (not sure why isort is putting unittest into wrong position)

Please see #1681 which will fix that issue.

…orts (`from shapely import xyz`) and run `isort`

Co-authored-by: Brendan Ward <[email protected]>
@jorisvandenbossche jorisvandenbossche changed the title BUG:PEP8: replace relative imports (i.e. from . import xyz) with absolute imports (from shapely import xyz) CLN: use absolute imports throughout the codebase Jan 30, 2023
@idanmiara
Copy link
Contributor Author

@brendan-ward please approve this PR as we are addressing your requested change in #1743 and #1681

@idanmiara
Copy link
Contributor Author

@jorisvandenbossche @brendan-ward
I think we are getting close with #1681 so I'm waiting for #1681 to be merged so I can rebase this PR - as there would probably be more conflicts once it's merged.

@jorisvandenbossche
Copy link
Member

Sorry, I'm not sure I follow what's the advantage of using relative imports (You noted that some other popular projects use them, but why is it an advantage? many other popular projects use absolute imports exclusively).

I was just giving those example to show that shapely is not some kind of oddity in using relative imports. It's not bad practice, it's just a subjective preference on development workflows (PEP8 recommends absolute imports, but also says that explicit relative ones are an "acceptable alternative").

But consistency within a code base is of course important.

I'm waiting for #1681 to be merged so I can rebase this PR - as there would probably be more conflicts once it's merged.

Given that #1681 also includes updating the unittest import, I removed the changes to the /tests subdirectory in this PR, then there might not be conflicts to resolve.

@jorisvandenbossche jorisvandenbossche merged commit 6cbae5d into shapely:main Feb 13, 2023
@jorisvandenbossche
Copy link
Member

Thanks @idanmiara

@idanmiara idanmiara deleted the abs_imports branch February 13, 2023 09:55
@idanmiara
Copy link
Contributor Author

@jorisvandenbossche thanks for your review!

@mwtoews
Copy link
Member

mwtoews commented Feb 13, 2023

I'm late to say thanks, looks good @idanmiara ! I generally prefer absolute imports within modules and appreciate consistency. A modern tool of interest is https://github.com/MarcoGorelli/absolufy-imports

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.

5 participants