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

feat(changelog): make changelog.header a template #698

Merged
merged 3 commits into from
Jun 17, 2024

Conversation

Cyclonit
Copy link
Contributor

@Cyclonit Cyclonit commented Jun 14, 2024

Made changelog.header into a template like changelog.body and changelog.footer. Added test test-header-template.

Description

Implement #697.

Motivation and Context

Create parity between changelog.header, changelog.body and changelog.footer.

How Has This Been Tested?

  • Added new test fixture "test-header-template".

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (no code change)
  • Refactor (refactoring production code)
  • Other

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.
  • I have formatted the code with rustfmt.
  • I checked the lints with clippy.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@Cyclonit Cyclonit requested a review from orhun as a code owner June 14, 2024 14:27
@Cyclonit Cyclonit force-pushed the feature/make_changelog_header_a_template branch 2 times, most recently from 9f53d67 to a50d1c9 Compare June 14, 2024 14:43
@Cyclonit
Copy link
Contributor Author

Cyclonit commented Jun 14, 2024

Hi @orhun,
functionally this is a very small change. I was able to copy the logic from the body and footer template processing 1-to-1. However, I do fear that we will have to flag this as a breaking change regardless. All of the tests and examples contain explicit trailing \n in the header. These are now obsolete. Without removing them, we would end up with an extra empty line between the header and the first release.

@Cyclonit Cyclonit force-pushed the feature/make_changelog_header_a_template branch 2 times, most recently from 2af1749 to 1363703 Compare June 14, 2024 15:00
@Cyclonit
Copy link
Contributor Author

Cyclonit commented Jun 14, 2024

I am not quite sure what to do about the allegedly wrong formatting in changelog.rs. When I run cargo fmt locally, it formats the code the way it is in this PR. I don't know why the pipeline demands it to be formatted differently. See https://github.com/orhun/git-cliff/actions/runs/9518131251/job/26238303168?pr=698

Similarly I am having trouble figuring out why the test suite fails: https://github.com/orhun/git-cliff/actions/runs/9518131251/job/26238304948?pr=698 Actual and expected look identical to me.
There were two errand tabs in the line after # Changelog. Took quite some time to notice them.

@Cyclonit Cyclonit force-pushed the feature/make_changelog_header_a_template branch 4 times, most recently from 2aff152 to c98f85c Compare June 14, 2024 15:30
@Cyclonit
Copy link
Contributor Author

And this is another problem: https://github.com/orhun/git-cliff/actions/runs/9518517603/job/26239610418?pr=698#step:3:842

I have no idea why the template behaves differently in test-header-template compared to test-footer-template. They are identical, but in test-header-template there is indentation, in test-footer-template there is not. I am done for today, maybe someone else could take a look into this.

@Cyclonit Cyclonit force-pushed the feature/make_changelog_header_a_template branch from c98f85c to 4033895 Compare June 14, 2024 16:03
@codecov-commenter
Copy link

codecov-commenter commented Jun 14, 2024

Codecov Report

Attention: Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.

Project coverage is 36.42%. Comparing base (dabe716) to head (17540a0).
Report is 1 commits behind head on main.

Files Patch % Lines
git-cliff-core/src/changelog.rs 72.73% 3 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #698      +/-   ##
==========================================
+ Coverage   36.29%   36.42%   +0.13%     
==========================================
  Files          19       19              
  Lines        1455     1461       +6     
==========================================
+ Hits          528      532       +4     
- Misses        927      929       +2     
Flag Coverage Δ
unit-tests 36.42% <72.73%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Cyclonit Cyclonit force-pushed the feature/make_changelog_header_a_template branch 5 times, most recently from 2abd2a8 to 35c185c Compare June 15, 2024 12:26
website/docs/templating/context.md Outdated Show resolved Hide resolved
git-cliff-core/src/changelog.rs Outdated Show resolved Hide resolved
git-cliff-core/src/changelog.rs Outdated Show resolved Hide resolved
git-cliff-core/src/changelog.rs Outdated Show resolved Hide resolved
git-cliff-core/src/changelog.rs Outdated Show resolved Hide resolved
git-cliff-core/src/changelog.rs Outdated Show resolved Hide resolved
git-cliff-core/src/changelog.rs Outdated Show resolved Hide resolved
git-cliff-core/src/changelog.rs Show resolved Hide resolved
@orhun
Copy link
Owner

orhun commented Jun 15, 2024

Can you resolve the conflicts? 😊

@Cyclonit Cyclonit force-pushed the feature/make_changelog_header_a_template branch 5 times, most recently from c32e7eb to b819e84 Compare June 16, 2024 06:11
Copy link
Owner

@orhun orhun left a comment

Choose a reason for hiding this comment

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

LGTM! Can you update the tests and documentation?

@Cyclonit Cyclonit force-pushed the feature/make_changelog_header_a_template branch 2 times, most recently from 408a4e2 to 66ba023 Compare June 17, 2024 14:31
@Cyclonit
Copy link
Contributor Author

I've fixed the tests and added the minimalist documentation update. How do we deal with the formatting? I cannot get my local rustfmt to format the code the way the pipeline wants it to be.

@orhun
Copy link
Owner

orhun commented Jun 17, 2024

I've fixed the tests and added the minimalist documentation update. How do we deal with the formatting? I cannot get my local rustfmt to format the code the way the pipeline wants it to be.

Thanks! The formatting is already fixed in main, so if you merge those changes it will be fine I think.

I can also handle it before merging the PR, no biggie.

@Cyclonit Cyclonit force-pushed the feature/make_changelog_header_a_template branch from 66ba023 to ea9841e Compare June 17, 2024 15:25
@Cyclonit
Copy link
Contributor Author

I've fixed the tests and added the minimalist documentation update. How do we deal with the formatting? I cannot get my local rustfmt to format the code the way the pipeline wants it to be.

Thanks! The formatting is already fixed in main, so if you merge those changes it will be fine I think.

I can also handle it before merging the PR, no biggie.

I've tried improving the documentation by specifying the context of each template. Is this okay?

Copy link
Owner

@orhun orhun left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

One thing that crossed my mind is that --prepend won't work as expected now when there are template variables in the header. It used to work like:

1- Read the contents from existing changelog file.
2- Replace the header in the file with the new changelog entries.

Now that the header is not a raw string & is not rendered, we won't be able to perform the replace properly.

Are you interested in fixing this as well? I think we only need to render header before replacing it in the file. I would really appreciate if you can test this case and create an issue!

@orhun orhun merged commit 9fc12bb into orhun:main Jun 17, 2024
50 checks passed
Copy link

welcome bot commented Jun 17, 2024

Congrats on merging your first pull request! ⛰️

@Cyclonit
Copy link
Contributor Author

LGTM, thank you!

One thing that crossed my mind is that --prepend won't work as expected now when there are template variables in the header. It used to work like:

1- Read the contents from existing changelog file. 2- Replace the header in the file with the new changelog entries.

Now that the header is not a raw string & is not rendered, we won't be able to perform the replace properly.

Are you interested in fixing this as well? I think we only need to render header before replacing it in the file. I would really appreciate if you can test this case and create an issue!

I was not aware of this feature. I created a bug issue to deal with that: #712

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.

3 participants