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(compiler): add dtx format support #28

Merged
merged 2 commits into from
Apr 15, 2018

Conversation

mugabe
Copy link
Contributor

@mugabe mugabe commented Apr 14, 2018

No description provided.

@coveralls
Copy link

coveralls commented Apr 14, 2018

Coverage Status

Coverage increased (+0.008%) to 98.547% when pulling 4eb7cd3 on mugabe:dtx-support into 42e81da on bemusic:master.

Copy link
Member

@dtinth dtinth left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. 😄

Is the only difference between DTX and BMS format is the space after the colon?

If so, in my opinion, we can use the same matcher by making BMS parser also allow extra space.

@mugabe
Copy link
Contributor Author

mugabe commented Apr 14, 2018

Not only space, there also colon after header name.
Ofcourse it could be solved by one regexp too, but for me it seems much correct to split it to different matchers.

@dtinth
Copy link
Member

dtinth commented Apr 14, 2018

I see, so that’s more than just whitespace issue. Thanks for the clarification!

Copy link
Member

@dtinth dtinth left a comment

Choose a reason for hiding this comment

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

Can you address this comment? Then it should be ready to merge :)

var matcher = matchers.bms

if (options.format === 'dtx') {
matcher = matchers.dtx
Copy link
Member

@dtinth dtinth Apr 14, 2018

Choose a reason for hiding this comment

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

To avoid reassigning more variables than necessary, I recommend compacting these five lines into one:

  var matcher = matchers[options.format] || matchers.bms

@dtinth
Copy link
Member

dtinth commented Apr 14, 2018

I missed this one: There is also a documentation block at the top of the function. Can you add the format parameter documentation too?

@mugabe
Copy link
Contributor Author

mugabe commented Apr 14, 2018

Done

Copy link
Member

@dtinth dtinth left a comment

Choose a reason for hiding this comment

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

Thanks!!

@dtinth dtinth merged commit 0243b5b into bemusic:master Apr 15, 2018
@dtinth dtinth removed the c:backlog label Apr 15, 2018
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.

3 participants