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

Relative imports #35

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

tonyfast
Copy link

@tonyfast tonyfast commented Sep 9, 2018

This is a cool project @SylvainDe .

This pull request uses relative imports to make didyoumean importable.

I wasn't able to pass all the tests because I think there are some issues with didyoumean with windows paths. I'm opening this PR to start testing.

Copy link
Owner

@SylvainDe SylvainDe left a comment

Choose a reason for hiding this comment

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

Oh, thank you so much! I'll have a look at this and integrate this as soon as possible!

@SylvainDe
Copy link
Owner

From what I can see in the Travis logs, there is a problem in the line with STAND_MODULES, can we remove it from the time being and improve it later on if need be?
Otherwise set union needs to be used. (Test case 'test_set_operation' is already written if I add such a suggestion ;-))

@Zwork101
Copy link

We should just use sys.builtin_module_names, it's exactly what we need. I see no reason to try and conjoin STAND_MODULES with it, just set the variable to it.

@SylvainDe
Copy link
Owner

SylvainDe commented Jan 15, 2019

@zwork it may be the case. I can't remember the reason behind this. There were probably a few subtle differences between the content of the variable and what I actually wanted. The difference can most probably be seen by triggering the unit tests after changing that part. I'll try to do it next time I have a few spare minutes in front of a computer and I'll update/comment the code of relevant. (Feel free to submit a PR to change this if you want to have the results sooner rather than later).
Thanks for your interest

@Zwork101
Copy link

Alright, I thought that had to do with the fact I was on windows, but I guess not. This PR isn't meant to solve that issue however, it's meant to add relative imports. I suggest we revert the variable back to it's original state and merge the relative imports.

@SylvainDe
Copy link
Owner

SylvainDe commented Jan 16, 2019 via email

@SylvainDe
Copy link
Owner

@tonyfast Do you want to handle the follow-up for this patch ? Do you mind if someone else does it based on your initial change ?

@SylvainDe
Copy link
Owner

@Zwork101 I've triggered a build to hilight the exact reason why we do not use sys.builtin_module_names: https://travis-ci.org/SylvainDe/DidYouMean-Python/builds/490268206 . Once Travis has run, you can have a look at the unit tests results. I can help you for an interpretation of the failures if need be.

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.

3 participants