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.

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

fix(changelog): fix missing commit fields in context (#837) #920

Merged
merged 1 commit into from
Oct 17, 2024
Merged

fix(changelog): fix missing commit fields in context (#837) #920

merged 1 commit into from
Oct 17, 2024

Conversation

dqkqd
Copy link
Contributor

@dqkqd dqkqd commented Oct 13, 2024 •
edited
Loading

Description

Add new fields raw_message in Commit to avoid lossy message conversion.

Fix #837

Motivation and Context

I implemented according to #837 (comment)

How Has This Been Tested?

Add 2 test fixtures

  • Create changelog with all fields from conventional commits
  • Run git cliff --context | git cliff --from-context should yield identical result with git cliff

Screenshots / Logs (if applicable)

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.

@dqkqd dqkqd requested a review from orhun as a code owner October 13, 2024 01:51
@welcome Welcome
Copy link

welcome bot commented Oct 13, 2024

Thanks for opening this pull request! Please check out our contributing guidelines! ⛰️

@codecov-commenter
Copy link

codecov-commenter commented Oct 13, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 50.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 41.49%. Comparing base (da1cb61) to head (6236060).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
git-cliff-core/src/commit.rs 50.00% 5 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #920      +/-   ##
==========================================
- Coverage   41.57%   41.49%   -0.07%     
==========================================
  Files          21       21              
  Lines        1677     1685       +8     
==========================================
+ Hits          697      699       +2     
- Misses        980      986       +6
Flag Coverage Δ
unit-tests 41.49% <50.00%> (-0.07%) ⬇️

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.

@dqkqd dqkqd force-pushed the issue-837 branch 2 times, most recently from a693040 to 52712d8 Compare October 13, 2024 01:58
@dqkqd dqkqd marked this pull request as draft October 13, 2024 02:16
@dqkqd dqkqd force-pushed the issue-837 branch 2 times, most recently from 134d053 to 9dab742 Compare October 13, 2024 06:50
@dqkqd dqkqd marked this pull request as ready for review October 13, 2024 06:57
orhun
orhun reviewed Oct 13, 2024
View reviewed changes
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.

Neat!

Left some comments :) Also, can you update the documentation (in /website) about the existence of the new raw_message field?

git-cliff-core/src/commit.rs Show resolved Hide resolved
git-cliff-core/src/commit.rs Outdated Show resolved Hide resolved
.github/workflows/test-fixtures.yml Outdated Show resolved Hide resolved
@dqkqd dqkqd force-pushed the issue-837 branch 2 times, most recently from 04e064c to f24e48d Compare October 13, 2024 12:35
@dqkqd
Copy link
Contributor Author

dqkqd commented Oct 13, 2024 •
edited
Loading

@orhun

I fixed according to your comments.
I also added the new field to context.md, but I'm not sure are there any other places I should look into.

orhun
orhun reviewed Oct 14, 2024
View reviewed changes
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.

One last thing :)

.github/fixtures/test-from-context-does-not-discard-fields/commit.sh Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

These test commits should no longer be needed. Can you test with an empty commit.sh and commit it if it works?

P.S. we should do the same for test-from-context fixture too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, it didn't work. I got an error, we must at least have one commit. I think this is a bug, but maybe we should fix this in another PR?

ERROR git_cliff > Git error: `reference 'refs/heads/main' not found; class=Reference (4); code=UnbornBranch (-9)`

However, even if we don't use commit.sh, I think we should just keep it there, as a reference. I find it more difficult to read context.json compared to commit.sh

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, okay. Yup, we need to fix that bug in another PR.

I find it more difficult to read context.json compared to commit.sh

Fair enough!

check.md Outdated Show resolved Hide resolved
@dqkqd dqkqd force-pushed the issue-837 branch 4 times, most recently from 367ea95 to 3d96357 Compare October 14, 2024 20:39
@dqkqd
fix(changelog): fix missing commit fields in context (orhun#837)
6236060 
* feat(commit): add `raw_message` to `Commit`

* test(fixtures): add test generate all fields in conventional commits

* test(fixtures): add test do not discard missing fields in conventional
commits when reading from context

* docs(website): add `raw_message` fields to `context.md`
orhun
orhun approved these changes Oct 17, 2024
View reviewed changes
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.

This is great, thanks for your contribution!

@orhun orhun merged commit f8641ee into orhun:main Oct 17, 2024
65 checks passed
@welcome Welcome
Copy link

welcome bot commented Oct 17, 2024

Congrats on merging your first pull request! ⛰️

@orhun orhun mentioned this pull request Oct 17, 2024
Closed
1 task
@janbuchar
Copy link
Contributor

Hi @orhun, I just encountered this issue. Could you please make a release that includes this fix?

@orhun
Copy link
Owner

orhun commented Nov 2, 2024

Yup, I will try to do a release next week.

@janbuchar
Copy link
Contributor

Yup, I will try to do a release next week.

Sorry to bother you, but any chance you could release it?

@Moisan Moisan mentioned this pull request Nov 21, 2024
Merged
@orhun
Copy link
Owner

orhun commented Nov 23, 2024

released now :)

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

@orhun orhun orhun approved these changes

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

Successfully merging this pull request may close these issues.

--from-context doesn't include footers
4 participants
@dqkqd @codecov-commenter @janbuchar @orhun