-
-
Notifications
You must be signed in to change notification settings - Fork 31
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
Add reminder to add changelog entry to pull request template #88
Add reminder to add changelog entry to pull request template #88
Conversation
|
||
- [ ] I added an entry to `CHANGELOG.md` if knowledge of this change could be valuable to users. | ||
- [ ] I linked any relevant issues from https://github.com/maplibre/maplibre-navigation-ios/issues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is what was meant by the previous "Open Tasks" heading, but feel free to correct me if I'm wrong here. I wasn't really sure what it meant. Or we can just get rid of it.
d9d6cb1
to
7018dc2
Compare
I've incorporated the discussion from our monthly maplibre-navigation call. Please take a look. |
|
||
- [ ] I added an entry to `CHANGELOG.md` if knowledge of this change could be valuable to users. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, maybe it would be worth while to also add a checkbox if the PR does not contain relevant changes? So contributors could select either one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's worth adding another checkbox to the checklist - the existing text covers this if they read carefully.
I'm kind of struggling to improve upon what I already have. But if you want to re-write the text for both of these options, I wouldn't mind using it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good, I think the current version is fine as well :)
I didn't include everything from https://github.com/maplibre/maplibre-navigation-ios/pull/86/files In particular, I omitted things that were fixups to the unreleased "Start & Stop Navigation in existing Map" And things that were test fixes, changes to the readme, and otherwise more likely to be distracting than useful for the user to know about.
7018dc2
to
256371f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
As an alternative to #86 , let's remind people to decide if a changelog entry is warranted as part of their PR process, rather than automatically generating an entry for every PR in the changelog.
This simple checklist approach has worked pretty well for me in practice for other projects.
Ultimately it's a judgement call between the PR author and the reviewer(s) if a changelog entry is warranted, but I expect the average outcome will be better this way vs. cluttering the changelog with updates that are not likely to be relevant to upgrading users.