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

- fixes #24: allow custom sections #39

Closed
wants to merge 1 commit into from
Closed

- fixes #24: allow custom sections #39

wants to merge 1 commit into from

Conversation

silkentrance
Copy link

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
Copy link
Author

Rebased to develop.

@silkentrance
Copy link
Author

Still need to update the README

@silkentrance
Copy link
Author

README updated

@mischah
Copy link
Collaborator

mischah commented Jan 10, 2015

Thanks for the PR. That’s a cool addition 👍

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
Copy link
Author

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
Copy link
Collaborator

mischah commented Jan 10, 2015

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 😕

@mischah
Copy link
Collaborator

mischah commented Jan 10, 2015

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

@silkentrance
Copy link
Author

Removed

@mischah
Copy link
Collaborator

mischah commented Jan 10, 2015

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

@silkentrance
Copy link
Author

Yes

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

/*

  • Needed to Foo for testing custom_sections Bar
    */

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
Copy link
Author

I have added the comment to Gruntfile.js

As for the m option...

@mischah
Copy link
Collaborator

mischah commented Jan 10, 2015

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

@silkentrance
Copy link
Author

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
Copy link
Collaborator

mischah commented Jan 10, 2015

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 😯
And I guess it’s not a use case like you already mentioned before.

@mischah
Copy link
Collaborator

mischah commented Jan 10, 2015

Thanks for your effort with this nice addition 😍

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

@silkentrance
Copy link
Author

@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
Copy link
Author

@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. 👍

@mischah mischah mentioned this pull request Jan 20, 2015
@ericmatthys
Copy link
Owner

@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
Copy link
Author

Current state of affairs:

  • instead of using /gim flags on the existing pre-defined regexps, this matches line by line
  • the existing regexps all match on line begin to end, i.e. ^...$ but with /gim in place they will match additional content

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

@silkentrance
Copy link
Author

@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
Copy link
Author

Rebased to master

@silkentrance
Copy link
Author

Collapsed existing commits into one.

@silkentrance
Copy link
Author

Rebased to develop

@silkentrance
Copy link
Author

Updated readme and collapsed commits.

@silkentrance
Copy link
Author

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

@silkentrance
Copy link
Author

Closing since I no longer have need for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants