Skip to content
This repository was archived by the owner on Nov 30, 2024. It is now read-only.

Dup formatters to prevent mutation #1486

Merged
merged 3 commits into from
Apr 16, 2014
Merged

Dup formatters to prevent mutation #1486

merged 3 commits into from
Apr 16, 2014

Conversation

JonRowe
Copy link
Member

@JonRowe JonRowe commented Apr 14, 2014

Counterpart to #1483 this prevents mutation of the formatter array.

@@ -643,7 +643,7 @@ def default_formatter=(value)

# @private
def formatters
Copy link
Member

Choose a reason for hiding this comment

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

I think we should make this method public and doc it. Without it being public there's no way to query RSpec to ask what formatters are being used. The doc can mention that a dup is returned.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

@@ -641,9 +641,16 @@ def default_formatter=(value)
formatter_loader.default_formatter = value
end

# @private
# @api public
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary? I would expect YARD to treat it as public anyway...

(Don't hold off merging on this...I'm mostly just curious).

Copy link
Member Author

Choose a reason for hiding this comment

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

Er, force of habit?

Copy link
Member Author

Choose a reason for hiding this comment

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

I pruned it with a [skip ci] ;)

@myronmarston
Copy link
Member

LGTM, merge when green.

JonRowe added a commit that referenced this pull request Apr 16, 2014
Dup formatters to prevent mutation
@JonRowe JonRowe merged commit 7f3ace2 into master Apr 16, 2014
@JonRowe JonRowe deleted the return_formatters_dup branch April 16, 2014 00:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants