Skip to content

Implement Reader class to parse ChangeLog.md #216

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

Merged
merged 10 commits into from
Mar 26, 2015
Merged

Implement Reader class to parse ChangeLog.md #216

merged 10 commits into from
Mar 26, 2015

Conversation

estahn
Copy link
Contributor

@estahn estahn commented Mar 25, 2015

This will fix "Parsing of existing Change Log file".
Fix #212.

@skywinder
Copy link
Member

@estahn you implement really handy module! 👏
Thanks! I will review it soon.

def parse_heading(heading)
structures = [
/^## \[(?<version>v.+?)\]\((?<url>.+?)\)( \((?<date>.+?)\))?$/,
/^## (?<version>v.+?)( \((?<date>.+?)\))?$/,
Copy link
Member

Choose a reason for hiding this comment

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

@estahn Most of all project uses prefix "v" for version tags, but not all of them.
As example of big projects, that don't use:

and this project also don't use any prefix for the tag versions.

So, I think it's needed to make char "v" optional in this regex. What do you think?

@estahn
Copy link
Contributor Author

estahn commented Mar 26, 2015

@skywinder I changed the parser to allow any kind of version number. Also added a bunch of rspec tests and rubocop to check for validity.

Travis is currently failing because most of the files are not compliant with rubocop. You could just run rubocop --auto-correct <filename> to fix the issues automatically.

Thoughts?

@skywinder
Copy link
Member

@estahn thanks for rspec and fixes! Much appreciated!

Tell the truth I'm new in ruby. Ans it's my first serious module-based project. And you are motivate me to do it well with your commits!

  • To not mix different tasks in one issue and make it clean - I will create separate PR with rspec integration. Please, merge it to your branch.
  • I think this parser - is good point to start implement tests! Thanks again!
  • As I can see, most of libs use module, named options.rb to implement options parse logic. I wrongly name in by parser.rb. But think that parser.rb is more suitable for your module. So, I think it will be better to rename parser.rb -> options.rb and reader.rb -> parser.rb. What do you think?

@skywinder skywinder merged commit 3ca25bf into github-changelog-generator:master Mar 26, 2015
@skywinder
Copy link
Member

Merged. Thanks for your big effort to cleanup and prettify this project! 👍
I muted all the rubocop's warnings, that can't be fixes automatically. I will eliminate them later one-by-one.

@skywinder skywinder mentioned this pull request Aug 4, 2015
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.

Parsing of existing Change Log file
2 participants