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

Updated CHANGELOG.md for 2.4 beta #4564

Merged
merged 3 commits into from
Dec 22, 2021
Merged

Updated CHANGELOG.md for 2.4 beta #4564

merged 3 commits into from
Dec 22, 2021

Conversation

daschuer
Copy link
Member

This is the updated CHANGELOG

After this is merged I think we can proceed to the beta phase.
Any thoughts?

@daschuer daschuer changed the title Updated CHANGELOG.md Updated CHANGELOG.md for 2.4 beta Dec 11, 2021
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@ronso0
Copy link
Member

ronso0 commented Dec 11, 2021

Thank you!
I did not yet do a thorough check, but I spotted some typos.

Let's discuss the beta topic in Zulip.

@ronso0
Copy link
Member

ronso0 commented Dec 11, 2021

I'm confused: do we want to use past tense for all changes from now on? Feels a bit weird...
Added harmonic keywheel window
vs.
Add harmonic keywheel window

@daschuer
Copy link
Member Author

I have not thought much about it. Now I see that we have the imperative in mots of the entries since now.
I will change the new entries to imperatives as well.

CHANGELOG.md Outdated Show resolved Hide resolved
[lp:1856402](https://bugs.launchpad.net/mixxx/+bug/1856402)
[lp:1944565](https://bugs.launchpad.net/mixxx/+bug/1944565)
* Keep selected track when browthing though the library, save model state [#4177](https://github.com/mixxxdj/mixxx/pull/4177)
* Allow to remove a track form the disk [#3212](https://github.com/mixxxdj/mixxx/pull/3212)
Copy link
Member

Choose a reason for hiding this comment

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

It's unclear if we should keep this feature, see discussion in #4560
I'd appreciate if you take look at this issue and share your opinion there.

Copy link
Member

Choose a reason for hiding this comment

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

If we keep it: "from"

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Thanks, some comments.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated
* Sync Lock: Fix issues with single-playing syncables
[#4155](https://github.com/mixxxdj/mixxx/pull/4155)
[#4389](https://github.com/mixxxdj/mixxx/pull/4389)
* Fix out of sync after scratching [#4005](https://github.com/mixxxdj/mixxx/pull/4005)
Copy link
Member

Choose a reason for hiding this comment

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

What is out of sync?

Copy link
Member

Choose a reason for hiding this comment

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

This refers to Sync Lock. Now if two tracks are synced, and the user scratchs one, sync is restored after scratching ends.

Copy link
Member

Choose a reason for hiding this comment

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

This should be reflected in the changelog entry.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
* Move contribution guidelines into our git repository [#2699](https://github.com/mixxxdj/mixxx/pull/2699)
* Automize deployment of CHANGELOG to the manual
* Moved contribution guidelines into our git repository [#2699](https://github.com/mixxxdj/mixxx/pull/2699)
* Automate deployment of CHANGELOG to the manual
Copy link
Member

Choose a reason for hiding this comment

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

Dont think this belongs into the changelog

[#4201](https://github.com/mixxxdj/mixxx/pull/4201)
[#4202](https://github.com/mixxxdj/mixxx/pull/4202)
* Update Benchmark library to v1.6.0 [#4540](https://github.com/mixxxdj/mixxx/pull/4540)
* Migration to Qt6 (work in progress)
Copy link
Member

@Holzhaus Holzhaus Dec 12, 2021

Choose a reason for hiding this comment

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

Do we really need to mention every single PR here? The list is quite long. Maybe we can just link the GH project?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at the final rendering, it becomes a single line like this:

Migration to Qt6 (work in progress) #4052 #4295 #4293 #4294 #4291 #4290 #4300 #4302 #4289 #4292 #4299 #4051 #4303 #4305 #4304 #4306 #4308 #4309 #4322 #4373 #4371 #4375 #4378 #4381 #4380 #4376 #4379 #4372 #4377 #4387 #4391 #4392 #4395 #4397 #4396 #4402 #4405 #4394 #4404 #4401 #4400 #4403 #4407 #4399 #4406 #4420 #4415 #4417 #4419 #4416 #4418 #4433 #4434 #4441 #4445 #4446 #4444 #4436 #4437 #4440 #4430 #4435 #4443 #4439 #4442 #4438 #4449 #4451 #4453 #4478 #4479 #4506 #4556 #4554 #4555 #4552 #4549

So this is kind of OK for now as a work in progress solution. It would work for me to list the single PRs somewhere else or drop the whole list at all.

Copy link
Member

Choose a reason for hiding this comment

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

thats still a really long line, I'd also prefer to just link the project.

Copy link
Member Author

Choose a reason for hiding this comment

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

The project is a moving target though. Since the whole topic does not really has relevance for the user in a 2.4 release, we can also consider to ditch the whole line.
I propose to postpone the decision to the final polish of this file before the actual release.
OK?

Copy link
Member

Choose a reason for hiding this comment

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

I think we want to keep the changelog focused on user-visible changes, and because this work isn't visible to users yet, I would argue for not including the line at all. The eventual changelog line we want is "Added support for QT6" and then we'd link to the project. Since it's such a major undertaking, it doesn't seem helpful to include every PR. (These are just my thoughts, I don't have a specific requirement for this part)

Co-authored-by: Jan Holthuis <holthuis.jan@googlemail.com>
Co-authored-by: ronso0 <ronso0@mixxx.org>
@daschuer
Copy link
Member Author

Don't think this belongs into the changelog

In general I agree or don't mind to all of these comments. But we have a workflow issue here and the current state is my attempt to work around this:

For some PR it is obvious that it is a user facing issue, for others it is obvious that is is an internal think only. But there are many PRs in the gray zone. If I had dropped them initially, a reviewer has to crawl the complete PR list to verify that all dropped PRs where dropped reasonably.

Here I have grouped all of these PRs under a common headline, to make them visible. I like to keep this state for now until we are about to do the final release. Then we have the whole picture in sight and can drop the not relevant PRs in an informed way.

Does this work for you?

* Sync Lock: Fix issues with single-playing syncables
[#4155](https://github.com/mixxxdj/mixxx/pull/4155)
[#4389](https://github.com/mixxxdj/mixxx/pull/4389)
* Re-sync to leader after scratching [#4005](https://github.com/mixxxdj/mixxx/pull/4005)
Copy link
Member

Choose a reason for hiding this comment

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

I was just going to suggest this change -- thanks

@ywwg
Copy link
Member

ywwg commented Dec 15, 2021

I would say if there's any gray area, don't include it in the changelog. I believe the biggest purpose of this file is for deciding what changes to report to users in social media and press reports, and there's usually only enough room for 10 headline features. I don't think it has to even include every user-visible change.

I will also defer mostly to the person who has volunteered to maintain the file! It's a thankless job so I don't want to make too many demands.

@daschuer
Copy link
Member Author

So we can merge this now as a WIP state? The advantage is that we have a version with all PRs in the git history for reference.

@ronso0
Copy link
Member

ronso0 commented Dec 18, 2021

👍
We can clean up and remove unneeded entries when we do the release.

Copy link
Member

@ywwg ywwg left a comment

Choose a reason for hiding this comment

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

approving for WIP, thanks for making these additions

@Holzhaus
Copy link
Member

So we can merge this now as a WIP state? The advantage is that we have a version with all PRs in the git history for reference.

Thanks for working on the changelog. I think it's unfortunate that our prior discussion if we want to assign the changelog label to a particular PR or not was a waste of time, because now we add every PR anyway.

This time we can merge the PR of course, but IMHO we should get our process sorted out. I don't think it makes much sense to discuss a process for maintaining the changelog if we then decide to ignore instead of following it.

I would say if there's any gray area, don't include it in the changelog.

I agree.

I believe the biggest purpose of this file is for deciding what changes to report to users in social media and press reports, and there's usually only enough room for 10 headline features. I don't think it has to even include every user-visible change.

I think the purpose of the changelog is to list user-visible changes and bugfixes, so that people who miss a feature or encountered a bug can check if it was fixed by a new release, but also to make cool new changes discoverable. It's not supposed to be a full list of every single change, that's what the git commit history is for. In the past, we sometimes included a phrase like "For a full list of new features and bugfixes, check out the 2.0.0 milestone on Launchpad." at the end of the changelog. We could do that again and link to the GitHub milestone if we think it's necessary.

I will also defer mostly to the person who has volunteered to maintain the file! It's a thankless job so I don't want to make too many demands.

I think it's actually straightforward now because we developed a process to ensure that nobody has to dig through the git history and decide which PR is worthy to mention months later. We have a "changelog" label that we add to all PRs that should be part of the changelog. Later we can just list the PRs that still need to be added to the changelog, make the necessary additions and remove the label.

@daschuer
Copy link
Member Author

I think the purpose of the changelog is to list user-visible changes and bugfixes

Yes, right.

My expectation is also, that it is "complete" in that regards.

The issue was here, that it turns out that, the changelog label process does not work, because it was not maintained. There where many relevant PRs without such a lable. So I have decides to go step by step through all PRs. I have added a meaningful description to all user-visible changes and bugfixes. All the others I have added to a group topic. From this state we can easly strip down the number of entries. The opposite approach of adding skipped PRs or with missing label later is IMHO not feasible.

I do not expect that the "changelog" label process will ever work well. This is because when you do a PR you have not the measure what is user relevant and what not. For my impression this measure also changes from release to release.
In a point release, small changes are more important for promoting a release, compared to a major release where such changes will fall under the not important noise PRs.

From this point of view this PR is a good intermediate state.

I can imagine that a set of changelog labels can improve the situation. Something like "Changelog Yes/Maybe/No"
This way, instead of reviewing the Changelog file, we can review the list of added and dropped PRs.
All PRs without a label can be labeled at any time later. However, I don't think this will speed up the work, because you cannot longer work in a single text file, and iron the descriptions along the way.

@Holzhaus
Copy link
Member

The issue was here, that it turns out that, the changelog label process does not work, because it was not maintained. There where many relevant PRs without such a lable.

Can you point to an example? Because I remember that the changelog label was regularily discussed with other team members whether we think a PR deserves a mention or not.

I do not expect that the "changelog" label process will ever work well. This is because when you do a PR you have not the measure what is user relevant and what not. For my impression this measure also changes from release to release. In a point release, small changes are more important for promoting a release, compared to a major release where such changes will fall under the not important noise PRs.

It's not only the responsibility of the PR author, but also of the person hitting the merge button. it's also known if the PR goes into a major or minor release beforehand, when the PR is merged, so this can be considered easily.

I can imagine that a set of changelog labels can improve the situation. Something like "Changelog Yes/Maybe/No"
This way, instead of reviewing the Changelog file, we can review the list of added and dropped PRs.
All PRs without a label can be labeled at any time later. However, I don't think this will speed up the work, because you cannot longer work in a single text file, and iron the descriptions along the way.

At first, we attempted to always add changelog entries as part of each PR. Unfortunately, this leads to merge conflicts that need to be resolved after merging each PR.

Hence, we decided we want to tag PRs using labels and add the changelog entry text to the PR description, so we can add changelog entries in bulk later on, which avoids the merge conflicts.

If you want to change this process, we can discuss it on Zulip. As I said, my concerns don't prevent merging this, but I think if we want to change our changelog process, we should discuss it and then make a decision as a team, instead of just ignoring it.

@daschuer
Copy link
Member Author

The issue was here, that it turns out that, the changelog label process does not work, because it was not maintained. There where many relevant PRs without such a lable.

Can you point to an example? Because I remember that the changelog label was regularily discussed with other team members whether we think a PR deserves a mention or not.

That's true, but we did not consider every relevant PR, that's why we have only 10 labeled:
https://github.com/mixxxdj/mixxx/pulls?q=is%3Amerged+is%3Apr+milestone%3A2.4.0+label%3Achangelog

If you want to change this process, we can discuss it on Zulip.

This PR was initially not the attempt to change the process, or intentional to ignore it. It was just not possible to follow the process. But I really wanted to proceed to beta 2.4, that's why I have made this work.

@Holzhaus
Copy link
Member

The issue was here, that it turns out that, the changelog label process does not work, because it was not maintained. There where many relevant PRs without such a lable.

Can you point to an example? Because I remember that the changelog label was regularily discussed with other team members whether we think a PR deserves a mention or not.

That's true, but we did not consider every relevant PR, that's why we have only 10 labeled: https://github.com/mixxxdj/mixxx/pulls?q=is%3Amerged+is%3Apr+milestone%3A2.4.0+label%3Achangelog

We didn't exactly have a lot of user-visible changes to main since we decided to use the label.

If you want to change this process, we can discuss it on Zulip.

This PR was initially not the attempt to change the process, or intentional to ignore it. It was just not possible to follow the process. But I really wanted to proceed to beta 2.4, that's why I have made this work.

Yes, okay. Sorry if I jumped to conclusions. I was just trying to avoid redundnt work in the future (like maintaining the changelog label and still digging through the commit history). However, this discussion shows that you do have concerns about the label, so we should probably discuss it on Zulip.

Sorry for derailing this PR discussion.

@Holzhaus Holzhaus merged commit ff2161d into mixxxdj:main Dec 22, 2021
@daschuer daschuer deleted the changelog branch May 4, 2022 09:56
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.

None yet

5 participants