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

add an option to create annotated tags #22

Merged
merged 13 commits into from
May 11, 2021
Merged

Conversation

rzabini
Copy link
Contributor

@rzabini rzabini commented Apr 17, 2021

In some cases it may be required to create annotated release tags, this PR adds this option to the Gradle extension.

Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

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

Thanks, this is a great contribution! The tricky part is that annotated tags have messages, and this PR always sets them to "". That's okay, but the natural next step is to make those tag messages configurable. I'm okay merging without full support for that, but I do want to make sure that we can add full support without breaking back-compat.

I left specific comments on the two places which I think need to change. I'll outline a broader configurable tag message design too, which could be in this PR or it can wait indefinitely for a future PR.

@FunctionalInterface
public interface TagAnnotation {
  public String message(Changelog.VersionEntry entry)

  public static TagAnnotation message(String msg) {
    return entryUnused -> msg;
  }
}

public class ChangelogExtension {
  TagAnnotation tagCfg = TagAnnotation.message(null);
 
  public void annotateTags(TagAnnotation tagCfg) {
    this.tagCfg = Objects.requireNonNull(tagCfg);
  }

  public void annotateTags(boolean annotateTags) {
    annotateTags(TagAnnotation.message(annotateTags ? "" : null));
  }
  ...

This would allow people to do things like:

spotlessChangelog {
  annotateTags({ "Release v$it.version() on $it.date():\n\n$it.changes()" })

@rzabini
Copy link
Contributor Author

rzabini commented Apr 23, 2021

I thought that the "annotated tag" feature is similar to what is already in place for the commit message, so I restructured the code following the same pattern. I changed the variable from a boolean to a String: an empty string means "create a lightweight tag", while any other value creates an annotated tag, provided that the message contains the version placeholder, the same as the commit message.
In addition, the field is now in GitCfg, instead of ChangelogExtension, so the method tagBranchPush in GitActions can be reverted to the no-args version, avoiding the change in the public interface.

Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

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

Ah, you are definitely correct that just a template string is better than an overly complex function. More consistent with commitMessage too.

But it is valid to want a heavyweight tag with an empty message (heavyweight tags have author info, lightweight don't), so it's important to make that possible.

After thinking about it a bit, the default behavior should probably be for the tag message to be the changes in that release. That would play nicely with the GitHub releases functionality (#10). This would be a breaking a change, but it's easy for users to opt-out:

### Added
- **BREAKING** release tags now have a message set to the changes in that release.
  - Previously, all tags were lightweight tags with no message.
  - To restore the previous behavior, specify `tagMessage null`
  - Tou can set any other message with `tagMessage '{{version}} blah blah {{changes}}'`

README.md Outdated Show resolved Hide resolved
rzabini and others added 2 commits May 1, 2021 18:32
Copy link
Contributor Author

@rzabini rzabini left a comment

Choose a reason for hiding this comment

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

Fine for me.

@nedtwigg
Copy link
Member

Sorry this took so long to merge. It had a bug where the tag was getting the changes entry for the previous version, rather than the correct one. But that was hard to fix because there was a different longstanding bug due to mutability in Changelog.releaseUnreleased(). I'm confident it's fixed now. My plan is:

  • release this as a new version
  • try it
  • maybe make it the new default (breaking change) depending on how well it works with GitHub releases.

@nedtwigg nedtwigg merged commit 9773e85 into diffplug:main May 11, 2021
@nedtwigg
Copy link
Member

Published in 2.2.0. Unfortunately it doesn't integrate with GitHub releases as well as I had hoped it might. Here's an example tag it generated: https://github.com/diffplug/blowdryer-diffplug/releases/tag/4.0.3

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.

2 participants