-
Notifications
You must be signed in to change notification settings - Fork 216
Make all imports absolute #3764
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
Comments
Hello, turns out this is super easy with ruff. # This is considered ok
from .benchmark_base import Benchmark, BenchmarkStudy
# rather than
from spikeinterface.benchmark.benchmark_base import Benchmark, BenchmarkStudy
# This is considered confusing
from ..preprocessing.astype import astype
# and would be changed to
from spikeinterface.preprocessing.astype import astype Note: we have 1279 same-level relative import and only 194 deeper-than-one-level. Should we keep the "simple" same-level relative imports? Opinions? @samuelgarcia @h-mayorquin @zm711 @JoeZiminski @yger ? |
Mmm, not sure here. I would vote for all absolute, but I agree that same level imports are less confusing! |
I agree with Alessio, but my only thought is if we can convince Sam to allow us to use ruff (see my PR #3756 for how easy linting is to add and how quick it is), then I would use whatever default is convenient so that it is all done in the pre-commit/commit action. In that case I would sacrifice the full consistency in order to have the automaticity. :) |
The Ruff pre-commit can handle both options with ease; the default is to allow for same-level relative imports. |
Okay that sounds like the best then to help new developers find their way around the library so I'm good with that. Thanks for playing around with it. I'm good with going all in on absolute then! |
I am fine with same-level relative imports. I am +1 on whatever gets this through, both options are better than what we have for me. |
Done in #3766 |
As we discussed, we should make all imports absolute throughout the source code
The text was updated successfully, but these errors were encountered: