8000 feat: Add configurable changelog omission for custom commit types by the-wondersmith · Pull Request #288 · cocogitto/cocogitto · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: Add configurable changelog omission for custom commit types #288

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Jun 23, 2023
Merged

feat: Add configurable changelog omission for custom commit types #288

merged 8 commits into from
Jun 23, 2023

Conversation

the-wondersmith
Copy link
Contributor
@the-wondersmith the-wondersmith commented Jun 8, 2023 •
edited
Loading

PR enables configurable changelog omission by commit type, as requested in #214

NOTE: implemenation is fairly naive, but is a completely functional "first draft" implementation

@the-wondersmith the-wondersmith mentioned this pull request Jun 8, 2023
Closed
@codecov
Copy link
codecov bot commented Jun 8, 2023 •
edited
Loading

Codecov Report

Merging #288 (4dea994) into main (bf38fa6) will decrease coverage by 0.03%.
The diff coverage is 90.90%.

@@            Coverage Diff             @@
##             main     #288      +/-   ##
==========================================
- Coverage   85.33%   85.31%   -0.03%     
==========================================
  Files          45       45              
  Lines        6207     6239      +32     
==========================================
+ Hits         5297     5323      +26     
- Misses        910      916       +6
Impacted Files Coverage Δ
src/conventional/commit.rs 92.82% <89.65%> (-0.98%) ⬇️
src/conventional/changelog/release.rs 99.81% <100.00%> (+<0.01%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bdarcus
Copy link
bdarcus commented Jun 9, 2023

@the-wondersmith - how would it work in terms of config?

@the-wondersmith
Copy link
Contributor Author
8000

@bdarcus If you look at the additions, it's fairly simple. I've added a field to conventional/commit.rs called omit_from_changelog. The new field defaults to false and is marked with #[serde(default)] both to ensure that there's no conflict with existing cog.toml files and to make it (effectively) optional. Unless explicitly configured otherwise, all commits will appear in generated changelogs - in other words the current behavior is preserved, the new behavior is explicitly opt-in.

CommitConfig::new's calling signature requires changelog_title to be supplied, meaning it shouldn't (really can't, technically) be changed without a considerable amount of forethought. Also, changing that would likely qualify as a breaking change to the internal API. To maintain the general usability though I've included two new methods to CommitConfig (omit and include) that allow for toggling the new flag on and off, in case that ever becomes necessary in the future.

Looking through the "flow" of things when changelogs are being generated, the least disruptive place I could see to add a check for whether or not a given commit should be omitted that wouldn't also result in cascading changes to various calling signatures / return types, I settled on CommitRange::from and implemented the check such that it simply declines to append any commit that's configured as omitted to the vec of commits being assembled in that method.

That specific choice of "shim" location necessitated some way to check if a Commit should be omitted, but I couldn't find any direct / explicit link between any given Commit and its associated CommitConfig until I saw how they're linked up in the Settings impl. Once I found that, I just sorta... extrapolated from there and added a Commit::should_omit method that allows a Commit to check itself against the global Settings and determine whether or not it should be omitted from changelog assembly 🙂.

Putting it all together, I updated the cog.toml for another project of mine to define a wip commit type that's not included in changelogs:

[commit_types]
hotfix = { changelog_title = "Hotfixes" }
release = { changelog_title = "Releases" }
wip = { changelog_title = "Work In Progress", omit_from_changelog = true }

Then generated changelogs -

before
after

@bdarcus
Copy link
bdarcus commented Jun 9, 2023

Looks good from that POV then!

I'm a rust newbie and amateur programmer, so don't feel competent to comment on the code.

@oknozor
Copy link
Collaborator
oknozor commented Jun 9, 2023

Nice work @the-wondersmith ! I will do a proper review on monday. Could you fix the lints and formatting until then ?

@the-wondersmith
Copy link
Contributor Author
the-wondersmith commented Jun 10, 2023 •
edited
Loading

@oknozor Of course. I pushed fixes for the linting issues, and clippy passes locally as well. llvm-cov has a few tests failing on my local machine (which is mildly concerning 🤔), but nothing failed when actions ran the test suite previously, so I'll have to assume that's some weird local quirk.

Please let me know if there's anything else I can do - eager for your review and/or thoughts otherwise 😁.

EDIT:

I'm 90% sure the failing tests are due to silliness in my local environment, so I'm not going to fret over them. I added support for the no_coverage attribute when running llvm-cov on nightly, (per the llvm-cov docs) to hopefully mitigate the minor dip in coverage that codecov reported. Looking that over though, the truly relevant parts of my additions are 100% covered so hopefully there shouldn't be an issue on that front.

Either way, let me know if I can do anything else.

@oknozor
Copy link
Collaborator
oknozor commented Jun 12, 2023

Hey @the-wondersmith I think failing tests are related to #235

That said you should write at least one test to ensure commit are omitted from changelog in cog_tests/changelog.rs.

@the-wondersmith
Copy link
Contributor Author

@oknozor test added.

Also you are 100% correct - tests are failing locally due to the exact situation described by #235 😅

@the-wondersmith
Copy link
Contributor Author

@oknozor Saw that everything was in the green except for coverage of the new omit and include utility methods. Presumably the coverage doesn't run on nightly, so the attributes aren't applied.

Reverted the addition of no_coverage support and added a simple test for the two that ensures that the flag behaves as advertised. Should be green across the board now (whenever everything runs).

@the-wondersmith
Copy link
Contributor Author

@oknozor Shameless self-bump

@oknozor
Copy link
Collaborator
oknozor commented Jun 17, 2023

@the-wondersmith there are still a few pending comments in re review

@the-wondersmith
Copy link
Contributor Author

@oknozor I may just be looking in the wrong place, but I can't seem to find any unaddressed comments (other than the ones that Codecov added to the test bodies, which I'd have thought llcov wouldn't have flagged given that they're inside test code, but either way shouldn't be of concern).

Can you point me in the right direction?

@oknozor
Copy link
Collaborator
oknozor commented Jun 20, 2023

@the-wondersmith look at the "Pending" comments here https://github.com/cocogitto/cocogitto/pull/288/files

@the-wondersmith
Copy link
Contributor Author

@oknozor all I've got on my end is:

no conversations

I absolutely don't want to presume, but is there any chance you haven't actually submitted the review(s) in question?

oknozor
oknozor reviewed Jun 20, 2023
View reviewed changes
@oknozor
Copy link
Collaborator
oknozor commented Jun 20, 2023

I did forgot to submit the review, oopsy :)

@cocogitto-bot
Copy link
cocogitto-bot bot commented Jun 20, 2023

✔️ 5a01106...4dea994 - Conventional commits check succeeded.

@the-wondersmith
Copy link
Contributor Author

@oknozor all requested changes made and pushed

@the-wondersmith
Copy link
Contributor Author

@oknozor We appear to be green across the board 😁

@oknozor oknozor merged commit 3ad69eb into cocogitto:main Jun 23, 2023
@oknozor
Copy link
Collaborator
oknozor commented Jun 23, 2023

Thanks a lot @the-wondersmith ! I will release as soon as I get the docs updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oknozor oknozor oknozor left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@the-wondersmith @bdarcus @oknozor
0