MNT Switch to absolute imports enforced by ruff#31847
MNT Switch to absolute imports enforced by ruff#31847jeremiedbb merged 6 commits intoscikit-learn:mainfrom
ruff#31847Conversation
ruff
sklearn/neighbors/_base.py
Outdated
| from sklearn.utils.validation import _to_object_array, check_is_fitted, validate_data | ||
|
|
||
| from ._ball_tree import BallTree | ||
| from ._kd_tree import KDTree |
There was a problem hiding this comment.
by default (ban-relative-imports = "parents") relative imports are allowed as long as they use a single . if I understand the doc. Maybe we want to use ban-relative-imports = "all"
I find it a bit weird to have a mix of relative and absolute, so I'm in favor of all absolute.
ogrisel
left a comment
There was a problem hiding this comment.
I find such absolute imports easier to mentally resolve, both when reading and editing code.
Furthermore, I like the fact that we now have a standard tool to check that we are always consistent in our import styles (contrary to the past where we had mixed import styles creeping for years and that required manual maintenance PRs to fix them at least on two occasions).
betatim
left a comment
There was a problem hiding this comment.
Fine for me.
I think we should not have the exception for "parent imports", use absolute imports everywhere.
The failed docs build looks like it would benefit from restarting
|
Scrolling through the diff and stopping at random-ish locations it doesn't seem like we are getting vastly longer lines. There are a few cases where one line imports turn into a multiline import but not very often (compared to how many lines are touched by this change) |
512221d to
38d3e94
Compare
|
Alright, let's merge with 4 approvals before accumulating conflicts. |
|
I fixed them all with:
Context: #31817 (comment)
Still TODO:
ban-relative-imports = "parents") relative imports are allowed as long as they use a single.if I understand the doc. Maybe we want to useban-relative-imports = "all"