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

Reject attempts to use hybrid Barclay/legacy syntax with the Barclay parser. #146

Merged
merged 1 commit into from
Nov 6, 2018

Conversation

cmnbroad
Copy link
Collaborator

Fixes #145.

@codecov-io
Copy link

codecov-io commented Oct 18, 2018

Codecov Report

Merging #146 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #146      +/-   ##
============================================
+ Coverage     76.56%   76.61%   +0.05%     
- Complexity      669      671       +2     
============================================
  Files            26       26              
  Lines          2296     2301       +5     
  Branches        455      455              
============================================
+ Hits           1758     1763       +5     
  Misses          366      366              
  Partials        172      172
Impacted Files Coverage Δ Complexity Δ
...titute/barclay/argparser/TaggedArgumentParser.java 85.85% <100%> (+0.75%) 24 <2> (+2) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 860ca67...f8cf671. Read the comment docs.

Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

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

@cmnbroad This looks good to me.

}
}
}

// Reject attempts to use hybrid Barclay/legacy syntax that contains embedded "=". Most of the time
// this works because jopt accepts "-O=value". But if "value" contains what appears to be tagging
Copy link
Member

Choose a reason for hiding this comment

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

Why not just use a javadoc comment? I've never really understood the prohibition on putting javadoc on private things.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just due to habit. Fixed.

@cmnbroad cmnbroad force-pushed the cn_fix_hybrid_syntax branch from b096758 to f8cf671 Compare November 6, 2018 19:24
@cmnbroad cmnbroad merged commit a08987a into master Nov 6, 2018
@cmnbroad cmnbroad deleted the cn_fix_hybrid_syntax branch November 6, 2018 19:28
@cmnbroad
Copy link
Collaborator Author

cmnbroad commented Nov 6, 2018

Thanks @lbergelson !

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