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

Rename pylint.Run do_exit parameter to exit #3554

Merged
merged 2 commits into from
May 1, 2020
Merged

Conversation

PCManticore
Copy link
Contributor

Description


Fix a crash in method-hidden lookup for unknown base classes (90d4215)

The patch replaces mro() with ancestors() as the former is not
fully capable of generating the complete linearization when
dealing with ambiguous inferences.

Close #3527


Revert pylint.Run's exit parameter to do_exit (a02b189)

This has been inadvertently changed several releases ago to do_exit.


Lint pylint from toxinidir, not the installed one (357922e)

Type of Changes

Type
🐛 Bug fix

Related Issue

Close #3533

This has been inadvertently changed several releases ago to ``do_exit``.

Close #3533
@coveralls
Copy link

coveralls commented May 1, 2020

Coverage Status

Coverage increased (+0.04%) to 90.431% when pulling 357922e on do-exit-should-be-exit into 90d4215 on 2.5.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 90.387% when pulling 357922e on do-exit-should-be-exit into 90d4215 on 2.5.

@PCManticore PCManticore merged commit 1654e49 into 2.5 May 1, 2020
@PCManticore PCManticore deleted the do-exit-should-be-exit branch May 1, 2020 07:04
@gangefors
Copy link

gangefors commented May 5, 2020

This change broke the pytest-pylint plugin.

INTERNALERROR>   File "/home/stefanga/projects/oe-fwrt/.tox/scripts/lib/python3.5/site-packages/pytest_pylint/plugin.py", line 174, in pytest_collection_finish
INTERNALERROR>     result = lint.Run(args_list, reporter=reporter, do_exit=False)
INTERNALERROR> TypeError: __init__() got an unexpected keyword argument 'do_exit'

Are they using your API wrong or was this a breaking change introduced in a patch release?

Edit: Now I see that it was changed from exit to do_exit and now back again. I suppose that the plugin maintainer will update to the expected API.

@gangefors
Copy link

gangefors commented May 5, 2020

After digging a bit. The do_exit change of the lint.Run() API was done as part of the work leading up to the 2.0 release. That means that the change from exit to do_exit was fair game since it was done in a major release.

What you have done now is broken an API that has been in the code since the 2.0 release. I would say that you should redact the 2.5.1 release removing it from PyPI and again revert back to the do_exit parameter name since that has been part of the 2.x API since the beginning.

# rant: The python community need to stop breaking things in minor/patch releases.

@PCManticore
Copy link
Contributor Author

@gangefors The 2.0 change from exit to do_exit was not intentional and it should not have been changed in the first place. I understand this causes some minor inconvenience, but I doubt it's so critical as you suggest.

Regarding your rant with the Python community, not sure from it is coming from. As long as you don't contribute to this project via code / doc / financial contributions please feel free to take your opinions elsewhere. This project has been developed on a voluntary basis, but it keeps getting "rants" from folks that only take but never give back.

@danielbrownridge
Copy link

Hooray - we have actively developed tools!
Hooray - people are moving fast and breaking things!
Hooray - people are finding bugs and making noise about them.
Hooray - people are working out the solutions.
Hooray - we can fix version numbers in our requriments.txt and Pipenv files if we need to.

Really appreciate everyone who has commented on this channel because the discussion means I can see that people who know what is going on are looking at things and that I wasn't going crazy. :-)

@PCManticore
Copy link
Contributor Author

@danielbrownridge @gangefors Just submitted #3591 which will allow both parameters to work in the meantime, until people can move to using exit instead. It will be removed in a future version. I was a frustrated by @gangefors rant but once I got some moments to clear my head, it made sense after all. This change should have been introduced in a minor release at best or deprecated as the given PR does. Regardless, sorry for causing any inconvenience, 2.5.2 should work properly once it's released.

@gangefors
Copy link

gangefors commented May 5, 2020

My rant was due to the fact that we spend a quite significant time debugging why our CI pipeline breaks with new versions of the packages we use (not this package specifically).
We do lock down the versions in our production pipe, but maintaining the development pipe where we want to move with the dependencies consumes a lot of time with issues like this.

Opening issues and troubleshooting where the issue stems from is a contribution by itself. Taking the additional time during business hours to work on a PR is not an option for me. And I see that the issue has been resolved.

Changing APIs should always be done with extreme care, especially when it comes to software used by millions.

I am thankful that you decided to deprecate the param instead of removing it immediately since this gives projects time to adapt. Thanks for the quick response!

niknetniko added a commit to dodona-edu/universal-judge that referenced this pull request Nov 24, 2023
Since pylint-dev/pylint#3554, do_exit has been
deprecated, so don't use it any more.
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.

4 participants