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

clean imports #898

Merged
merged 3 commits into from
Apr 28, 2021
Merged

clean imports #898

merged 3 commits into from
Apr 28, 2021

Conversation

asanin-epfl
Copy link
Contributor

@asanin-epfl asanin-epfl commented Apr 26, 2021

  • drop relative imports
  • delete unused PointType class
  • don't use private modules, rename _neuron to neuron, _soma to soma

delete unused PointType class
@asanin-epfl asanin-epfl requested review from tomdele and arnaudon April 26, 2021 17:01
@asanin-epfl
Copy link
Contributor Author

@tomdele Alexis's approval does not allow me to merge. You will have to look here.

Copy link
Contributor

@tomdele tomdele left a comment

Choose a reason for hiding this comment

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

looks good

@asanin-epfl
Copy link
Contributor Author

Thank you!

@asanin-epfl asanin-epfl merged commit 1a170f1 into master Apr 28, 2021
@asanin-epfl asanin-epfl deleted the imports branch April 28, 2021 12:10
@adrien-berchet
Copy link
Member

Hi @asanin-epfl
No big deal but this PR introduces breaking changes due to file renamings. It would have been nice to increase the minor version number instead of just the bugfix number ;-)

@asanin-epfl
Copy link
Contributor Author

asanin-epfl commented Apr 29, 2021

Yes! I totally forgot. Should not be critical as private modules become public.

@adrien-berchet
Copy link
Member

No problem, it is not critical indeed.

@asanin-epfl
Copy link
Contributor Author

Thank for the heads up!

@arnaudon
Copy link
Contributor

Wait, this could break a few other codes, no? No other complaints so far?

@asanin-epfl
Copy link
Contributor Author

It will break only if user imported the private module.

@arnaudon
Copy link
Contributor

ah ok ok , let see, it may just be me in diameter-synthesis then

@adrien-berchet
Copy link
Member

Not only since imports were removed from the neurom.core.__init__.
So all from neurom.core import * must be changed to from neurom.core.neuron import *. Are these considered as private?

@asanin-epfl
Copy link
Contributor Author

ok ok, 2.1.0

@adrien-berchet
Copy link
Member

Thanks :-)

@adrien-berchet
Copy link
Member

Now I am wondering if this change worth it. It actually breaks at least TNS, morph-tool and diameter-synthesis. Maybe reverting the imports in neurom/core/__init__.py would be simpler than updating everything else?

@asanin-epfl
Copy link
Contributor Author

ok

@asanin-epfl
Copy link
Contributor Author

what else besides neurom/core/__init__.py?

@adrien-berchet
Copy link
Member

Hard to check everything but I guess we shoud also revert neurom/view/__init__.py and neurom/io/__init__.py

@asanin-epfl
Copy link
Contributor Author

This one no neurom/io/__init__.py. It duplicates already neurom/__init__.py.

@adrien-berchet
Copy link
Member

But maybe another code was importing something like neurom.io.NeuronLoader instead of neurom.NeuronLoader so it would still be breaking for it. I don't think we have this kind of imports in TNS, diameter-synthesis and morph-tool so it should not bother me but maybe it's the case somewhere else so it's as you prefer.

@asanin-epfl
Copy link
Contributor Author

yes but in that case another code needs to change such import without changing the version of NeuroM in its setup.py

@adrien-berchet
Copy link
Member

Yeah true, it's ok for me then.

Copy link
Contributor

@tomdele tomdele left a comment

Choose a reason for hiding this comment

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

looks good

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.

4 participants