Skip to content

Allow override of identifier_in_use lambda#86

Merged
elrayle merged 1 commit intomasterfrom
fix/85_override_in_use_lambda
Nov 26, 2018
Merged

Allow override of identifier_in_use lambda#86
elrayle merged 1 commit intomasterfrom
fix/85_override_in_use_lambda

Conversation

@elrayle
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown

@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.

👍

Comment thread spec/unit/config_spec.rb
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
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