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: Added in support for Sprig Go Template Functions #75

Closed
wants to merge 44 commits into from

Conversation

delmendo
Copy link
Contributor

@delmendo delmendo commented Jun 4, 2020

What does this do / why do we need it?

This adds the template methods from Sprig to the git-chglog universe. The sprig library is a common toolkit for templates. I have used it a lot on the helm project for kubernetes.

I wanted to be able to deduplicate commit messages as well as do other formatting tasks that ship standard with sprig.

How this PR fixes the problem?

It just adds on the library and funcationality.

What should your reviewer look out for in this PR?

This is my first time contributing to a go project of any kind. I was able to get everything to compile and all tests pass on my machine. I'm still unfamiliar with the way things work in goland so please give me any feedback to

Check lists

  • [X ] Test passed
  • [X ] Coding style (indentation, etc)

Additional Comments (if any)

{Please write here}

Which issue(s) does this PR fix?

fixes #73
fixes #29

@coveralls
Copy link

Pull Request Test Coverage Report for Build 164

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 75.185%

Totals Coverage Status
Change from base Build 155: 0.0%
Covered Lines: 1630
Relevant Lines: 2168

💛 - Coveralls

@ghostsquad
Copy link
Collaborator

@delmendo can you rebase please?

@talbright
Copy link
Contributor

@delmendo this would be super helpful for users...can you clean this up so it can get merged in?

@delmendo
Copy link
Contributor Author

I'll carve out some time today to work on this.

@delmendo
Copy link
Contributor Author

Full disclosure - I know very little go - I only learned enough to modify this package to do most of what I wanted. I rebased and resubmitted. When I run the tests locally they pass - but there is something wrong with how the deps are being pulled over and I'm not sure where to start. If you could give me a pointer in what I'm doing wrong I'd be willing to put some more time in to try to get this to land

@talbright
Copy link
Contributor

talbright commented Feb 23, 2021

I do have a suggestion.

The commit that really matters here is only this one: 5f9300f

Try this:

  • Pull latest from upstream master into the master branch of your fork.
  • Create a new branch from the master branch of your fork that now has the latest from upstream master.
  • In the new branch cherry pick 5f9300f into it.
  • Run go mod tidy
  • Create a new PR

go mod tidy ensures that the go.mod file matches the source code in the module. It adds any missing module requirements necessary to build the current module's packages and dependencies, and it removes requirements on modules that don't provide any relevant packages. It also adds any missing entries to go.sum and removes unnecessary entries.

This will give you a cleaner base to work with, and should cleanup any dependency issues 🤞

@delmendo
Copy link
Contributor Author

Ok so it looks like I did a number of other modifications to my version of Chglog ( I needed better handling for squash commits and some other changes - so after the rebase I didn't think you wanted all that version included. I created a new fork and generated the merge form there so it can be handled cleaner. It looks like Sprig has had a major version update since I did this work so I updated the library to use that so it will be easier to main tain in the future.

Here is the new pull request - #99

let me know if there is anything else I need to do - or if you want the other changes I made to be submitted to the upstream. Glad to see this project is getting love again!

@talbright
Copy link
Contributor

Nice 👍

You should close this to cut down on the noise for the maintainers...

@delmendo delmendo closed this Feb 23, 2021
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.

Expect to filter duplicate commits! feature: add sprig functions to templete.FuncMap