Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants