ericmatthys / grunt-changelog

MIT License
45 stars 10 forks source link

- fixes #24: allow custom sections #39

Closed silkentrance closed 6 years ago

silkentrance commented 9 years ago

I have recreated the patch based on the new develop branch and fixed all test cases. I also included a new test case for the custom sections feature.

silkentrance commented 9 years ago

Rebased to develop.

silkentrance commented 9 years ago

Still need to update the README

silkentrance commented 9 years ago

README updated

mischah commented 9 years ago

Thanks for the PR. That’s a cool addition :+1:

I left some comments at line numbers. See above. Next thing I’m going to do ist checkout your changes on my local machine …

silkentrance commented 9 years ago

Updated README and included usage example

As for the m option: The input file is split on newline and each line is then processed on its own. I guess, rewriting the getChangelog/getChange routine would probably do the trick.

Your example commit message has the keyword closes along with the issue id on one line. Would you want to include all of the other information in the change log as well?

mischah commented 9 years ago

Your example commit message has the keyword closes along with the issue id on on line. Would you want to include all of the other information in the change log as well?

Not, not personally. But it would be possible with the logArguments option. But to be honest, I have no idea if people would like to generate a changelog that way :confused:

mischah commented 9 years ago

Question: I don’t see the need of test/fixtures/log_custom. Are you using that file?

silkentrance commented 9 years ago

Removed

mischah commented 9 years ago

Oops. My line comments are gone with your commit :laughing: To you still have them in your inbox?

silkentrance commented 9 years ago

Yes

Gruntfile.js@11: I would love to see a short comment in the style of:

/*

changelog.js@49

Bad idea to get rid of the m option! Because user may use the »keywords« within »new lines« of the commit like I use to do.

See also also changes of:

README.md Gruntfile.js of your commit.

silkentrance commented 9 years ago

I have added the comment to Gruntfile.js

As for the m option...

mischah commented 9 years ago

As for the m option: I’m doing some test by now :blush:

silkentrance commented 9 years ago

As for the m option... that is why I defaulted to --pretty=%B which will preserve the new lines instead of squashing %s the commit message into a single line

But I guess that keeping the /gim options for the original regexes would be a wise move in conjunction with --pretty=%s.

This would require two different code paths though, one for use with custom sections and a second for reproducing the old behaviour, which is actually lost with this patch when going against a git repository.

In my world, though, commit messages are made up of lines with those to keep prefixed by a - hyphen and those that should not be kept not being prefixed, e.g.

- miscellaneous information, e.g. bumped version to x.x.x
- fixes #12: no longer crashing on start
[ci skip]
also ignored
mischah commented 9 years ago

Using '--pretty=%B' kinda works to render multi line commit messages into the changelog. But it’s pretty ugly because you can’t recognize which line belongs to which commit message.

Example commit message:

Cleanup package.json
Delete unneeded properties `main`, `engines`.
Related to #20.

Will be become the following lines within the changelog:

- Cleanup package.json 
- Delete unneeded properties `main`, `engines`. 
- Related to #20. 

Which looks like three different commits.

You could reproduce that with the following settings within the grunt file

log_arguments: {
  options: {
    logArguments: [
      '--pretty=%B',
      '--no-merges',
      '--date=short'
    ],
    dest: 'tmp/changelog_logArguments',
    template: '[date]\n\n{{> features}}',
    after: '2014-04-08',
    before: '2015-01-10',
    featureRegex: /^(.*)$/i,
    partials: {
      features: '{{#if features}}{{#each features}}{{> feature}}{{/each}}{{else}}{{> empty}}{{/if}}\n',
      feature: '- {{{this}}} {{this.date}}\n'
    }
  }
},

But that seems to be already »broken«, before your PR :hushed: And I guess it’s not a use case like you already mentioned before.

mischah commented 9 years ago

Thanks for your effort with this nice addition :heart_eyes:

@ericmatthys Do you like to review this before I merge it into the develop branch?

silkentrance commented 9 years ago

@mischah @ericmatthys Since this breaks existing behaviour with the --pretty=%s when getting the log from git, I would refrain from merging this at this stage.

I am currently looking into xregexp, if you do not mind adding another dependency, and joining all regular expressions, making them named groups and using the flags ymi on all regular expressions, incrementally parsing the log instead of splitting it into distinct lines.

The named groups can in turn be used by the user in his or her section regexes, and will also be passed along with the data to the template in order for the user to define the templates in a more flexible way.

silkentrance commented 9 years ago

@mischah @ericmatthys xregexp is rather problematic and leads to a lot of unwanted infinite loops, besides of that it comes short when using equally named sub groups. dropping this approach.

Need to give up on this one, although I hate it.

As of %s this will only fail with my commit messages, thanks to logArguments this is now leveraged and I think that this can be safely merged. :+1:

ericmatthys commented 9 years ago

@mischah If you add comments through the Files tab instead of directly on the commit, the PR will still show your comments even if the commit it was attached to is force pushed out of existence.

It seems like multiline commit messages and custom sections are two different topics, which we don't want to conflate in this PR. I'd rather see custom sections implemented as close to the existing uses of regex / log parsing first, and then look at changing that behavior separately if we need to.

silkentrance commented 9 years ago

Current state of affairs:

I will revert these changes and make this match the original behaviour.

silkentrance commented 9 years ago

@mischah @ericmatthys I have revised the PR and reverted getChanges to its original behaviour, no longer line matching. I had to give up on my fallback 'others' section regexp, though, as it will now duplicate existing entries.

I need to figure this out, perhaps a multi-line option would do the trick, shifting existing parsing from global to single line...

silkentrance commented 9 years ago

Rebased to master

silkentrance commented 9 years ago

Collapsed existing commits into one.

silkentrance commented 9 years ago

Rebased to develop

silkentrance commented 9 years ago

Updated readme and collapsed commits.

silkentrance commented 9 years ago

@mischah Perhaps you might want to have a look at this?

silkentrance commented 6 years ago

Closing since I no longer have need for this.