SylvainDe / DidYouMean-Python

Module to have suggestions in case of errors (NameError, AttributeError, etc).
MIT License
120 stars 15 forks source link

Reformat test structure and imports #50

Closed TylerYep closed 2 years ago

TylerYep commented 4 years ago

Hi, I was interested in using this project, but ran into some issues with pip installing the package. I ended up renovating the project to fit my needs, and thought there was a chance you might be interested in using the changes.

Please feel free to take these changes or throw them away, I'm mainly just leaving this PR here as a suggestion of possible improvements!

Changes included:

SylvainDe commented 4 years ago

Thank you so much for your pull request! I remember that I struggled trying to make this installable with pip for a very limited success ( https://github.com/SylvainDe/DidYouMean-Python/issues/3 ).

I'll have a look at the details and integrate it as soon as possible.

SylvainDe commented 4 years ago

Note: I don't know why this does not appear more explicitly here but you can find the Travis runs for your Pull Request here: https://travis-ci.org/github/SylvainDe/DidYouMean-Python/builds/730786103 .

TylerYep commented 4 years ago

Are you dedicated to supporting Python 2.6 at this point? All versions of up to Python 3.5 have reached end-of-life by now (https://endoflife.date/python), so it seems wise to move along with this trend - especially if this project is not currently used by many people yet.

It would be pretty easy to split this into multiple changes - I can do it sometime this week. Thanks for reviewing the changes!

SylvainDe commented 4 years ago

This looks great! Thanks for the additional work you've gone through.

Unfortunately, I can see that the CI is failing - https://travis-ci.org/github/SylvainDe/DidYouMean-Python/pull_requests . Would you have any idea about what happened ?

As for the support for older Python version, I'd rather keep it until it does bring a lot more troubles while moving forward. If it is only for DeprecationWarning, I'd rather add a few wrappers around the unit-test methods to ensure that everything works smoothly in all cases.

SylvainDe commented 4 years ago

Hello @TylerYep , I've performed a few changes which will unfortunately lead to merge issues. On the positive side: the DeprecationWarnings are now fixed and I've removed a few pep8 errors so that Black can be used safely.

To you mind resubmitting your code organisation PR ? (If you want, I can perform it myself based on the work you've done and try to give you the proper attribution). Similarly, I can perform the call to Black in a different commit if you want.

Please let me know how you want to handle these and thanks for your work!

TylerYep commented 4 years ago

Resubmitted the PR. I can go ahead and create the PR for the black formatting, but could you fix the travis CI integration first? I believe you just need to:

As a side note, I would also really recommend upgrading to a newer toolchain for linting + testing. I believe the more common standard now is pytest + flake8 instead of unittest + pep8/pycodestyle/pyflakes.

Pytest should be able to execute all unittests without any modifications, but with a better test-discovery. The following commands should do everything correctly and concisely in the .travis.yml:

isort .
black .
flake8 .
pytest
codecov
SylvainDe commented 4 years ago

Thanks for all the suggestions ! It looks like I have not followed the latest trends on a few aspects indeed. I've disabled scrutinizer for all PRs. As for Travis, a few things changed (apps, migration from org to com) and I did not quite manage to make it work yet. I'll try again later on.