feat: add `--sort [TYPE]` flag #78

sanderblue · 2020-07-29T19:20:22Z

What does this do / why do we need it?

First off, thank you for creating an awesome tool. We're using git-chglog in many open source repositories at New Relic and we would like to continue doing so.

The PR introduces sorting tags by version number instead of sorting by the date of tag creation. Users can still sort by date by passing the flag --sort-by-date. Using --sort-by-date will perform the original behavior of the git-chglog command.

Per request to integrate with the latest configuration updates from #124 (nice addition @ldelossa 🙂 ), this pull request introduces sorting tags by semver version using the --sort [TYPE] flag where TYPE is either date or semver. Sort by date is still default behavior.

Previous reasoning behind the idea:
Since we support multiple major release branches of a repository, our CHANGELOG gets mangled when we release v1.x and then release an update for v2.x. Essentially the v1.x commits get combined with the v2.x commits in the CHANGELOG and requires manual editing that can be cumbersome with every new release we create. Sorting the tags by version number fixes this issue.

Give it a test drive:

make build

This will build the binaries for supported architectures located in the ./bin directory- e.g. ./bin/{arch}/git-chglog (replace {arch} with darwin, etc)

./bin/darwin/git-chglog

How this PR fixes the problem?

Sorting the tags by version number ensures git-chglog selects the correct previous tag to compare to the new tag.

Current hypothetical ordering for a CHANGELOG might look like this:

v2.1.0 <-- will contain commit messages from v1.1.1 due to how git-chglog sorts from tag timestamp
v1.1.1 <-- patch release for v1.x (this causes an issue for all v2.x release after this one)
v2.0.0
v1.1.0
v1.0.0

This is because the diff that's being checked is github.com/user/repo/compare/v1.1.1...v2.1.0, but we need this to be github.com/user/repo/compare/v2.0.0...v2.1.0 for the latest release in the example above.

This PR introduces ordering for a CHANGELOG that looks like this:

v2.1.0 
v2.0.0
v1.1.1 
v1.1.0
v1.0.0

This ensures the correct previous tag is selected for comparison to create the CHANGELOG entries.

What should your reviewer look out for in this PR?

This PR migrates this project from dep to Go modules. The vendor directory is no longer needed.

Check lists

Test passed
Coding style (indentation, etc)

Which issue(s) does this PR fix?

Resolves: #74
Resolves: #112

ctrombley · 2020-07-29T20:02:28Z

👍

zlesnr

Nice!

ghostsquad · 2020-08-09T19:23:34Z

yes please!

ghostsquad · 2021-01-10T07:16:37Z

@sanderblue can you rebase this so we can see if we can get the tests to run & pass? We have a few new maintainers/contributors to the project, so now is the time to kick this into high gear.

This is technically not backwards compatible, so maybe we should wait to merge this and add --sort-by-version flag in addition, and a warning that in a future version, --sort-by-version will be the default?

sanderblue · 2021-01-11T17:41:28Z

@ghostsquad good call on the flag idea. I should be able to address this relatively soon - hopefully within the next couple days.

coveralls · 2021-01-23T16:37:37Z

Pull Request Test Coverage Report for Build 695618253

0 of 0 changed or added relevant lines in 0 files are covered.
11 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.04%) to 76.76%

Files with Coverage Reduction	New Missed Lines	%
cmd/git-chglog/main.go	11	73.0%

Totals
Change from base Build 688745689:	0.04%
Covered Lines:	1843
Relevant Lines:	2401

💛 - Coveralls

khos2ow · 2021-01-23T18:56:58Z

This sounds really cool. To be frank I haven't actually looked at the changeset yet, but wanted to suggest converting the new flag from bool to string, i.e. using --sort TYPE (defaults to date if not present). Then we will have all the flexibility to add more sort types in the future without polluting the flags count.

sanderblue · 2021-01-30T17:23:05Z

Hi @khos2ow, thanks for the feedback, but that kind of feels like premature optimization at this stage and that would also add additional code complexity because some form of validation would be needed to ensure a valid type string was passed which could grow over time. Plus right now there are only two ways to sort and I'm not sure if there would be any other ways to sort that make sense here other than maybe ascending or descending (which could be by date or by version as well). That being said, if there's a use case I'm missing I'm all ears (or eyes in this case) 🙂

ghostsquad · 2021-02-07T21:09:01Z

I don't think it's that far fetched to provide a sort-by which takes a field/type or something along those lines. It seems fairly common.

Looking at other tools, it looks like gsort has a --sort flag that takes types but ALSO provides long & short sort-specific flags:

https://www.gnu.org/software/coreutils/manual/html_node/sort-invocation.html

‘-g’
‘--general-numeric-sort’
‘--sort=general-numeric’


‘-h’
‘--human-numeric-sort’
‘--sort=human-numeric’

‘-M’
‘--month-sort’
‘--sort=month’

‘-n’
‘--numeric-sort’
‘--sort=numeric’

‘-R’
‘--random-sort’
‘--sort=random’

trim21 · 2021-03-03T15:47:16Z

Is it possible to add a config option at the same time?

zlesnr · 2021-03-04T22:54:59Z

I think I agree with @sanderblue that this feels like premature optimization to include a sort string right now, but perhaps others could change that after merge? What other sorting methods would we implement other than the default behavior, or what this flag introduces? Also, 10 files changed, 3 commits, and the contributor included tests. This seems like a great addition to me. If other folks want a config option, perhaps that can be included after merge also.

clok · 2021-03-04T23:33:22Z

Just adding my take that the current work is great and the call outs a sort type and config sound like great iterations to extend this feature.

clok

Thanks for the work here! Like the sort by feature. Can you please rebase from master to pick up the changes to urfave/cli/v2. You will need to address one compatibility issue with the new CLI flag.

Also, I left a comment concerning the changes to the Makefile.

Awesome work!

clok · 2021-03-04T23:34:53Z

cmd/git-chglog/main.go

@@ -126,6 +126,12 @@ func CreateApp(actionFunc cli.ActionFunc) *cli.App {
 			Usage: "Regular expression of tag filter. Is specified, only matched tags will be picked",
 		},

+		// sort-by-version
+		cli.BoolFlag{


This will be needed after the rebase with current master and the bump to urfave/cli/v2

Suggested change

cli.BoolFlag{

&cli.BoolFlag{

Updated. Should be good to go now 😎

Thanks for the review as well!

clok · 2021-03-04T23:36:01Z

Makefile

@@ -20,3 +20,35 @@ install:
 .PHONY: changelog
 changelog:
 	@git-chglog --next-tag $(tag) $(tag)
+
+
+.PHONY: compile


Can these changes be moved out to a separate PR please? While potentially useful, they are not related to the addition of the sort by feature.

Sure thing, no worries :)

sanderblue · 2021-03-08T17:27:10Z

@clok I've updated the PR per latest feedback.

Thanks for all the additional reviews and feedback as well 🙂

clok

Thanks for the follow up. Dropped an in-line comment with a link to #112 that we can use to follow up with a more robust sort flag.

Appreciate the follow through and contribution.

clok · 2021-03-08T19:58:46Z

tag_reader.go

+	if r.sortByVersion {
+		err := r.sortTagsByVersion(tags)
+		if err != nil {
+			return nil, err
+		}
+	} else {
+		r.sortTagsByDate(tags)
+	}


Noting the addition of #112 to follow up with the support for a more robust sort feature.

clok · 2021-03-09T12:52:13Z

@mavogel @khos2ow can you take a look at this PR? I think it's ready to go.

khos2ow

Thank you @sanderblue, LGTM!

I think you need to rebase one final time on master to be able to proceed with this.

clok

@sanderblue we have merged #124 which provides this feature via a configuration option. This has been released in v0.12.0 as well.

While the work in that PR overlaps with the work you have done in this feature PR, we do see it as a potentially complementary feature. We would like to add the CLI flag --sort that can provide the functionality of the config value as well.

Would you like to rebase your feature branch and integrate the functionality from #124 with the CLI flag work you have done? Or would you like to have one us take on the work of introducing the --sort flag?

No pressure either way. We wanted to ask you first as you have put in this contribution and we appreciate your work. Thanks again!

sanderblue · 2021-03-25T00:44:38Z

@clok sounds good. I should be able to get this updated within the next day or so once I wrap up a couple things with work.

clok · 2021-03-25T04:03:38Z

@sanderblue thats great! Totally understand the delay. No worries there. Don't hesitate to reach out if you have questions or feedback. This work will resolve #112 as well. Thanks!

sanderblue · 2021-03-26T22:29:39Z

I've rebased and will be working to get this working soon with the latest updates.

sanderblue · 2021-03-27T00:10:41Z

@clok @khos2ow I have rebased and updated the code to use --sort [TYPE] as requested and it's ready for your review. This really slimmed down the pull request as well. Thanks for the patience 😎

clok

This looks great! Thanks again for keeping on this, following up on the feedback and updating after the config based PR came through. 🥇

If you could update the README with the updated CLI help output in the "CLI Usage" section, that would be awesome. Just helps us keep the repo up-to-date.

Leaving an "Approve" on this as I don't see it as critically blocking and I am happy to pick up that edit in the end.

clok · 2021-03-28T03:50:21Z

cmd/git-chglog/config.go

@@ -308,7 +308,7 @@ func (config *Config) Convert(ctx *CLIContext) *chglog.Config {
 		Options: &chglog.Options{
 			NextTag:                     ctx.NextTag,
 			TagFilterPattern:            ctx.TagFilterPattern,
-			Sort:                        opts.Sort,
+			Sort:                        orValue(ctx.Sort, opts.Sort),


Nice. This was what I was looking for. 😀

sanderblue · 2021-03-28T18:20:06Z

@clok updated the CLI usage section with the new --sort flag help output.

clok · 2021-03-29T03:26:47Z

This has been releases as a part of the v0.14.0 release. Thank you for your contribution!

sanderblue changed the title ~~feat(tag_reader): sort by tag version~~ feat: sort by tag version by default, add --sort-by-date flag for previous functionality Jul 29, 2020

sanderblue force-pushed the feat/sort-by-version branch 3 times, most recently from 10acacd to cf0ecc6 Compare July 29, 2020 19:40

zlesnr approved these changes Jul 29, 2020

View reviewed changes

sanderblue force-pushed the feat/sort-by-version branch from cf0ecc6 to 2bc7aff Compare January 23, 2021 16:36

sanderblue force-pushed the feat/sort-by-version branch from 2bc7aff to 3a3564a Compare January 23, 2021 18:21

sanderblue force-pushed the feat/sort-by-version branch from 3a3564a to c2eb966 Compare January 23, 2021 19:12

sanderblue changed the title ~~feat: sort by tag version by default, add --sort-by-date flag for previous functionality~~ feat: add --sort-by-version flag Jan 30, 2021

clok requested changes Mar 4, 2021

View reviewed changes

sanderblue force-pushed the feat/sort-by-version branch from 038d6cb to 346111a Compare March 7, 2021 22:19

sanderblue requested a review from clok March 8, 2021 03:17

clok mentioned this pull request Mar 8, 2021

Idea: --sort [TYPE] flag support #112

Closed

clok approved these changes Mar 8, 2021

View reviewed changes

khos2ow approved these changes Mar 10, 2021

View reviewed changes

clok mentioned this pull request Mar 18, 2021

feat: allow tag sorting by semver #124

Merged

2 tasks

clok requested changes Mar 21, 2021

View reviewed changes

sanderblue force-pushed the feat/sort-by-version branch 4 times, most recently from c71fd01 to 0ad8bbb Compare March 26, 2021 23:52

feat(chglog): add --sort flag

2f00715

sanderblue force-pushed the feat/sort-by-version branch from 0ad8bbb to 2f00715 Compare March 26, 2021 23:54

sanderblue changed the title ~~feat: add --sort-by-version flag~~ feat: add --sort [TYPE] flag Mar 26, 2021

sanderblue requested a review from clok March 28, 2021 00:48

clok approved these changes Mar 28, 2021

View reviewed changes

chore(sort): update README with --sort usage

a96d4c4

khos2ow self-requested a review March 28, 2021 20:54

clok merged commit e523fd4 into git-chglog:master Mar 29, 2021

sanderblue mentioned this pull request Apr 12, 2021

chore(build): update git-chglog now that --sort flag has been implemented newrelic/terraform-provider-newrelic#1273

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add `--sort [TYPE]` flag #78

feat: add `--sort [TYPE]` flag #78

feat: add --sort [TYPE] flag #78

feat: add --sort [TYPE] flag #78

Conversation

What does this do / why do we need it?

How this PR fixes the problem?

What should your reviewer look out for in this PR?

Check lists

Which issue(s) does this PR fix?

Choose a reason for hiding this comment

Pull Request Test Coverage Report for Build 695618253

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat: add `--sort [TYPE]` flag #78

feat: add `--sort [TYPE]` flag #78