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

Use relative instead of absolute import #968

Closed
samuelstjean opened this Issue Mar 13, 2016 · 14 comments

Comments

Projects
None yet
3 participants
@samuelstjean
Contributor

samuelstjean commented Mar 13, 2016

For anyone wanting to fix quick stuff, turning all of the relative import to absolute import would be useful to prevent name clashes when importing files from other projects or locally from the working folder.

@arokem

This comment has been minimized.

Member

arokem commented Mar 13, 2016

I am -1 on that. From PEP8: "Absolute imports are recommended, as they are usually more readable and tend to be better behaved". As I understand it, clashes like the ones you want to prevent will not happen with absolute imports, only with mis-formed relative imports (I think -- happy to see an example where I am wrong). Yet another reason to avoid relative imports -- it's easy to mess them up ("how many dots was I supposed to put in there?").

To further this discussion, these seem to be the compelling use-cases for relative imports: http://stackoverflow.com/questions/12738889/when-or-why-to-use-relative-imports-in-python

Do they clearly apply to Dipy?

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Mar 13, 2016

-1 here too. This has been in discussion in the past. The conclusion was that we will not change our imports to relative imports as the benefit doesn't worth the effort and there are also concerns that are expressed on the pep 8 and elsewhere. I think we should close this issue as it is not an issue and it has been addressed in the past.

@arokem

This comment has been minimized.

Member

arokem commented Mar 13, 2016

There does seem to be some non-uniformity on this, though. With some
modules having relative imports (e.g.,
https://github.com/nipy/dipy/blob/master/dipy/reconst/shore.py), and others
having absolute imports (e.g.,
https://github.com/nipy/dipy/blob/master/dipy/reconst/sfm.py). Some even
have a mixture (e.g.,
https://github.com/nipy/dipy/blob/master/dipy/reconst/dki.py).

So what is our general policy? "Use absolute imports, except where
necessary"?

Should we aim to fix up those places that have relative imports that are
not necessary, as part of our Spring cleaning? Maybe as a Sunday afternoon
diversion for one of our contributors? :-D

On Sun, Mar 13, 2016 at 12:34 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

-1 here too. This has been in discussion in the past. The conclusion was
that we will not change our imports to relative imports as the benefit
doesn't worth the effort and there are also concerns that are expressed on
the pep 8 and elsewhere. I think we should close this issue as it is not an
issue and it has been addressed in the past.


Reply to this email directly or view it on GitHub
#968 (comment).

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Mar 13, 2016

Not a crucial update but if someone wants to do it. Be my guest. It can be easily a beginner's issue :)

@arokem

This comment has been minimized.

Member

arokem commented Mar 13, 2016

Labeled as such.

@arokem arokem changed the title from Use relative instead of absolute import to Use absolute instead of relative import Mar 13, 2016

@arokem

This comment has been minimized.

Member

arokem commented Mar 13, 2016

And edited title :-)

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Mar 13, 2016

Not a good idea. Because the first post should change too. Otherwise it is misleading. I would close this issue and open another one.

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Mar 13, 2016

Indeed they are mixed everywhere, so I thought it was the other way around like nibabel did. Anyway, what would be the easiest way to chunk off parts of some modules to include in other subprojects then? I wanted to kind of borrow the sh fit routines, but it depends on so much stuff it is painful to remove uneeded stuff for 2-3 functions.

As it stands, wiht all the compiled code in the registration module, building things is on the long side when one wants only a few functions.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Mar 13, 2016

I disagree that it is so hard or painful to copy parts from modules to include to other projects. This just needs a bit of care. But it is far away from hard. So, the easiest way is to just do it Sam. Put the time to check the imports and what you really need and know what calls what. No free lunch here.

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Mar 13, 2016

You know you can just ask nicely for it ;)

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Mar 13, 2016

Sorry what do you mean?

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Mar 13, 2016

Yeah I tried and gave up midway, so whatever, I'll just keep having my local branch and an official version installed also, could not find out how to make setup.py understand I have a local version.

--> look at the first post

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Mar 13, 2016

Talled? So, the real problem is that you don't know how to write properly the setup.py file. Sorry this is not DIPY's issue. It seems you are mixing things together. I am closing this issue.

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Mar 13, 2016

No, I do know how it works, I just said having dipy as a whole dependency is overkill for me, so hence the issue. Then I realized relative import were the mistake instead of the norm, hence why someone changed the title, then I changed the first post to reflect that.

It all makes sense now, but just open a new one without edit and those random messages.

@arokem arokem changed the title from Use absolute instead of relative import to Use relative instead of absolute import Mar 13, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment