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

Update astroid dependency #4111

Closed
wants to merge 3 commits into from
Closed

Update astroid dependency #4111

wants to merge 3 commits into from

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Feb 21, 2021

Steps

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Add a ChangeLog entry describing what your PR does.
  • If it's a new feature or an important bug fix, add a What's New entry in doc/whatsnew/<current release.rst>.
  • Write a good description on what the PR does.

Description

Update required astroid dependency to astroid==2.5 for the next pylint release.
I've also added a short script to fetch the correct astroid hash during CI runs.

Why ==2.5 and not ~=2.5?
This is something I would like to discuss here further.
I believe that it might be beneficial to lock it to 2.5 instead of only requiring the minor version to match. Most users probably only define a pylint version in their test requirements. So when astroid pushes a new bugfix release, it won't be installed since the requirement is already satisfied. The solution would be to also release a small pylint update the updates the astroid dependency.

The only drawback I can think of is, that it might lead to conflicts if another project is also using astroid as a dependency but with a different version. Don't know how common that would be. I would imagine most use only pylint.

Type of Changes

Type
βœ“ πŸ› Bug fix
βœ“ ✨ New feature
πŸ”¨ Refactoring
πŸ“œ Docs

Related Issue

#4055 (comment)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.451% when pulling 4e29eeb on cdce8p:pin-astroid-2 into 3509e57 on PyCQA:master.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Agree. Another benefit we would get is that during developement pylint's CI would not break because something happened on astroid's master. We can make a merge request to upgrade astroid and while we fix it all MR for pylint can be merged without problem. We just had this problem where MR were accumulating on pylint's side while we were fixing astroid and I think defining astroid version in pylint is the solution.

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.7.0 milestone Feb 21, 2021
@Pierre-Sassoulas Pierre-Sassoulas added Enhancement ✨ Improvement to a component task labels Feb 21, 2021
@cdce8p
Copy link
Member Author

cdce8p commented Feb 21, 2021

Another benefit we would get is that during development pylint's CI would not break because something happened on astroid's master. We can make a merge request to upgrade astroid and while we fix it all MR for pylint can be merged without problem. We just had this problem where MR were accumulating on pylint's side while we were fixing astroid and I think defining astroid version in pylint is the solution.

That's another issue that isn't fixed at the moment. CI still uses the latest astroid commits
https://github.com/PyCQA/pylint/blob/4e29eeb22f6ccda01bf0f079201c5b47d10c48de/requirements_test_pypy.txt#L1
I would be fine however with changing that as well. It would simplify a few things in the CI config. The only downside would probably be, that we would need more frequent astroid releases to take advantage of it in the master branch. What's your opinion on it?

@Pierre-Sassoulas
Copy link
Member

I think it's better to be based on the tag, no ? acb6871

Yes we would have to release astroid more frequently and create a merge request in pylint, but I think the stability of the pylint's continuous integration is worth it. (Also having to try to reproduce with multiple astroid when a bug appear is not ideal)

@Pierre-Sassoulas
Copy link
Member

Can we merge that after the 2.7.0 release ? It seems it's about optimizing Github Actions, right ?

@cdce8p
Copy link
Member Author

cdce8p commented Feb 21, 2021

Can we merge that after the 2.7.0 release ? It seems it's about optimizing Github Actions, right ?

As long as the astroid update to 2.5 is included, this one can wait.

@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.7.0, 2.7.1 Feb 21, 2021
@cdce8p cdce8p mentioned this pull request Feb 21, 2021
4 tasks
@cdce8p
Copy link
Member Author

cdce8p commented Feb 21, 2021

Replaced by #4116

@cdce8p cdce8p closed this Feb 21, 2021
@cdce8p cdce8p deleted the pin-astroid-2 branch February 21, 2021 23:00
@Pierre-Sassoulas Pierre-Sassoulas added Maintenance Discussion or action around maintaining pylint or the dev workflow and removed task labels Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants