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

Support braced hashlike on FromString as documented. #14

Merged
merged 2 commits into from
Jul 14, 2018

Conversation

itsjamie
Copy link
Contributor

Fix #13.

@itsjamie itsjamie requested a review from acln0 July 14, 2018 20:02
Copy link
Member

@acln0 acln0 left a comment

Choose a reason for hiding this comment

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

LGTM. Waiting for @theckman to merge.

@coveralls
Copy link

coveralls commented Jul 14, 2018

Pull Request Test Coverage Report for Build 38

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.6%) to 98.137%

Totals Coverage Status
Change from base Build 26: 0.6%
Covered Lines: 316
Relevant Lines: 322

💛 - Coveralls

Copy link
Member

@theckman theckman left a comment

Choose a reason for hiding this comment

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

@itsjamie can you take a look at the coveralls report, and see if the tests can be tweaked to include the lines it claims aren't tested?

It seems like codec.go is the one it's not happy with:

@acln0
Copy link
Member

acln0 commented Jul 14, 2018

@theckman I think this isn't a big deal. Some of the things missing were not covered before, and will be covered by my larger testing overhaul PR, which I can send soon. The relevant new code paths introduced by this patch are covered, so I think it's fine to merge it as-is. Your call.

@itsjamie
Copy link
Contributor Author

itsjamie commented Jul 14, 2018

I can submit a change to cover the FromBytesOrNil and FromStringOrNil success cases.

The default switch case in decodePlain that is uncovered can't be reached from public API (as far as I can tell). Which raises the question, unit test private implementation or only what can be reached from accessible APIs?

Copy link
Member

@theckman theckman left a comment

Choose a reason for hiding this comment

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

@itsjamie thank you for the extra effort to get the test coverage back up! That looks awesome!

While I'd generally prefer this as a single commit, I'll merge with the two commits attached.

@theckman theckman merged commit 39ae215 into master Jul 14, 2018
theckman added a commit that referenced this pull request Jul 14, 2018
Support braced hashlike on FromString as documented.

Signed-off-by: Tim Heckman <t@heckman.io>
@acln0 acln0 mentioned this pull request Jul 17, 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.

4 participants