Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Allow override of identifier_in_use lambda #86

Merged
merged 1 commit into from
Nov 26, 2018

Conversation

elrayle
Copy link
Contributor

@elrayle elrayle commented Nov 26, 2018

Fixes #85

Uses ||= to allow for override of identifier_in_use lambda. See issue for more information.

@elrayle elrayle self-assigned this Nov 26, 2018
@coveralls
Copy link

coveralls commented Nov 26, 2018

Pull Request Test Coverage Report for Build 363

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 98.077%

Totals Coverage Status
Change from base Build 361: 0.0%
Covered Lines: 153
Relevant Lines: 156

💛 - Coveralls

@elrayle elrayle requested a review from jcoyne November 26, 2018 16:45
Copy link

@no-reply no-reply left a comment

Choose a reason for hiding this comment

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

👍

@@ -7,6 +7,7 @@
it { is_expected.to respond_to(:statefile) }
it { is_expected.to respond_to(:namespace) }
it { is_expected.to respond_to(:minter_class) }
it { is_expected.to respond_to(:identifier_in_use) }
Copy link
Member

@jcoyne jcoyne Nov 26, 2018

Choose a reason for hiding this comment

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

Isn't this a bit duplicative? You are explicitly testing this method below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't normally include this, but I was following the existing pattern. Methods template and minter_class are also explicitly tested and have the respond_to test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked with @jcoyne in Slack and he gave the thumbs up to merge.

Copy link
Member

@jcoyne jcoyne left a comment

Choose a reason for hiding this comment

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

Overall seems good. I just had a minor nit to pick about testing.

@elrayle elrayle merged commit 22597e7 into master Nov 26, 2018
@elrayle elrayle deleted the fix/85_override_in_use_lambda branch November 26, 2018 18:27
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants