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

Release 2.15.5 #7660

Merged
merged 11 commits into from
Oct 21, 2022
Merged

Release 2.15.5 #7660

merged 11 commits into from
Oct 21, 2022

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Oct 21, 2022

Description

My first try doing a release 😄

Cherry-picking worked fine. Ran into a few issues with the contributors-txt script and towncrier though.

  1. Running python3 script/create_contributor_list.py only adds new contributors to the bottom. Is that correct, are they ordered somehow?
  2. Should we add aliases even if they only have one email?
  3. python3 script/bump_changelog.py {new_version} changes the version in towncrier.toml. Should they be reset? I saw that this wasn't included in previous patch releases.
    https://github.com/PyCQA/pylint/blob/f324be7de7dd20ac7763954d98d831140c6d5a91/towncrier.toml#L2-L4

Which command do you use to merge the changes into main afterwards? Just git merge upstream/maintenance/2.15.x or something else?

danmou and others added 11 commits October 20, 2022 11:28
…values inferred (pylint-dev#7627)

Add a keyword-only `compare_constants` argument to `safe_infer`.
When the dictionary has served its purpose (making plugin loading
pre-and-post init a consistent behaviour), we swap it for bools
indicating whether or not a module was loaded.
We don't currently use the bools, but it seemed a sensible choice.
The main idea is to make the dictionary fully pickle-able, so that when
dill pickles the linter for multiprocessing, it doesn't crash horribly.

Co-authored-by: Daniël van Noord <[email protected]>
Co-authored-by: Marc Mueller <[email protected]>
@cdce8p cdce8p added Maintenance Discussion or action around maintaining pylint or the dev workflow Skip news 🔇 This change does not require a changelog entry labels Oct 21, 2022
@cdce8p cdce8p added this to the 2.15.5 milestone Oct 21, 2022
@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Oct 21, 2022

Running python3 script/create_contributor_list.py only adds new contributors to the bottom. Is that correct, are they ordered somehow?

The script is not doing it properly, I've been manually sorting contributors. tbh I did not find the time to fix it but someone else making the release and having to do the manual sort make the shame sting a little now. If we want to fix this it might be easier to move the additional descriptions to json and then recreate the full file from scratch instead of parsing it each time.

Should we add aliases even if they only have one email?

No we don't have too, only if there's duplicate and an error during generation. (i.e. two emails for one person)

Should they be reset? I saw that this wasn't included in previous patch releases.

I don't really know so I deselected this change because it's less thing to care about when merging the maintenance branch in main later (otherwise we need to revert it manually).

@Pierre-Sassoulas
Copy link
Member

Which command do you use to merge the changes into main afterwards? Just git merge upstream/maintenance/2.15.x or something else?

This command but things you also need to know:

  • the changelog is duplicated silently so in astroid you need to check the changelog for the next patch version
  • It's probably possible to fix conflict with a git checkout --ours but I feel the risk / reward ratio is really too high so I just fix conflict manually
  • I check the diff with git diff upstream/main before opening a PR (for pylint) or pushing (for astroid) the merge commit to main.

@cdce8p
Copy link
Member Author

cdce8p commented Oct 21, 2022

Which command do you use to merge the changes into main afterwards? Just git merge upstream/maintenance/2.15.x or something else?

This command but things you also need to know:

  • the changelog is duplicated silently so in astroid you need to check the changelog for the next patch version
  • It's probably possible to fix conflict with a git checkout --ours but I feel the risk / reward ratio is really too high so I just fix conflict manually
  • I check the diff with git diff upstream/main before opening a PR (for pylint) or pushing (for astroid) the merge commit to main.

👍🏻 I just tested the merging and it seem to work. At least the final diff is as expected. Had to resolve a few conflicts but was fairly strait forward.

Should they be reset? I saw that this wasn't included in previous patch releases.

I don't really know so I deselected this change because it's less thing to care about when merging the maintenance branch in main later (otherwise we need to revert it manually).

Having tested the back-merge, I think it's safe to include the change (in the maintenance branch). It will be resolved during the merge and even if not updated when the bump_changelog.py script is run.

Running python3 script/create_contributor_list.py only adds new contributors to the bottom. Is that correct, are they ordered somehow?

The script is not doing it properly, I've been manually sorting contributors. tbh I did not find the time to fix it but someone else making the release and having to do the manual sort make the shame sting a little now. If we want to fix this it might be easier to move the additional descriptions to json and then recreate the full file from scratch instead of parsing it each time.

It fine. We can add it to our long ToDo list 😄 I was just confused why it wasn't working / if it was working correctly since I didn't follow the discussions around it.

--

I think the release is ready then.

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.

Just FYI Daniel suggested to create a backporting merge request and then a subsequent release branch to be able to review the release easily, let's do it next time :)


- Sort ``--generated-rcfile`` output.

Refs #7655 (`#7655 <https://github.com/PyCQA/pylint/issues/7655>`_)
Copy link
Member

Choose a reason for hiding this comment

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

When copy pasting the release in github I currently have to remove the link manually (it's rst and github want markdown).

@cdce8p cdce8p merged commit bb17694 into pylint-dev:maintenance/2.15.x Oct 21, 2022
@cdce8p cdce8p deleted the release-2.15.5 branch October 21, 2022 21:16
@cdce8p
Copy link
Member Author

cdce8p commented Oct 21, 2022

Just FYI Daniel suggested to create a backporting merge request and then a subsequent release branch to be able to review the release easily, let's do it next time :)

Not exactly sure what you mean. E.g. I needed to redo #7662 to include the merge commit.
Or just open it for comparison, do everything else, rebase / redo the backporting merge request?

@Pierre-Sassoulas
Copy link
Member

Not exactly sure what you mean.

I meant opening a MR for the cherry-picking (backporting) in maintenance/2.15.x and another one for the release (tbump command) in maintenance/2.15.x before the release of 2.15.5. Like for #7591 / #7590.

That way it's easier to see what the tbump command did. It's the only thing that really need review at release time (if there wasn't any conflict during cherry-picking).

But, this is just a remark for next release: let's not redo everything for the sake of it, I do not mind reviewing everything in batch personally.

@cdce8p
Copy link
Member Author

cdce8p commented Oct 22, 2022

Not exactly sure what you mean.

I meant opening a MR for the cherry-picking (backporting) in maintenance/2.15.x and another one for the release (tbump command) in maintenance/2.15.x before the release of 2.15.5. Like for #7591 / #7590.

Think I got it now. So separating the backporting PR form the actual release. My impression was that it's easy enough to view just the last commit (the one from tbump) so it wouldn't be necessary. Additionally, IMO it's quite nice to have all commits regarding a patch release in one PR.

@Pierre-Sassoulas
Copy link
Member

Think I got it now. So separating the backporting PR form the actual release. My impression was that it's easy enough to view just the last commit (the one from tbump) so it wouldn't be necessary. Additionally, IMO it's quite nice to have all commits regarding a patch release in one PR.

What do you think @DanielNoord ?

@DanielNoord
Copy link
Collaborator

For pylint the PRs can get very large. Since cherry picked commits can still be completely different than the actual commit we preferably also check those to make sure nothing bad slipped in.
Together with the many changes in contributors and new changelog generation those PRs got a little too big to review easily.

@cdce8p
Copy link
Member Author

cdce8p commented Oct 22, 2022

For pylint the PRs can get very large. Since cherry picked commits can still be completely different than the actual commit we preferably also check those to make sure nothing bad slipped in.
Together with the many changes in contributors and new changelog generation those PRs got a little too big to review easily.

They are large either way. Splitting it into two doesn't reduce the line count. IMO it's fairly easy to separate the changelog and contributor changes during the review with commit ranges. E.g. https://github.com/PyCQA/pylint/pull/7660/files/9c239c2eecb5925a4aabc73423c65eb67fdf151d

Creating a second PR is just more work and will make it more difficult later to understand what the actual release was.

Since cherry picked commits can still be completely different than the actual commit we preferably also check those to make sure nothing bad slipped in.

That's always an issue with cherry-picking for patch releases since theoretically you could hide something just in the maintenance branch. In the end, I think the best solution here would be to release more often so the diffs are smaller. Besides that only a select few (at the moment maybe we three) should do them at all.

Just for reference, this is a PR for a Home Assistant patch release. Note that the last commit / the version bump is included. home-assistant/core#80691

@DanielNoord
Copy link
Collaborator

Yeah, agreed but this was mainly due to the issues we had with the new changelog, contributor list and copyright notices. We had to rebase and force push many times. In those cases it was much easier to just merge the commits first.
A changelog change can easily be reviewed from mobile, that also helps.

I mean if you want to keep doing them like this that is also fine with me of course!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow Skip news 🔇 This change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants