DidYouMean-Python

DidYouMean-Python copied to clipboard

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.

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 ;-))

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.

@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

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.

That's seems like the best option indeed. Once it's done, continuous integration will be happy and so will I. Thanks for your PR

In the meantime, I'll try to investigate why I didn't use the variable mentioned above and comment things accordingly.

Le jeu. 17 janv. 2019 à 00:14, Nate the great [email protected] a écrit :

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.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/SylvainDe/DidYouMean-Python/pull/35#issuecomment-454980459, or mute the thread https://github.com/notifications/unsubscribe-auth/ABL0rWdgqYCwkiIiqO2Bjm4kc6D2xcrDks5vD7I_gaJpZM4WgIDn .

@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 ?

@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.