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

Benchmark of transfer_nested_constants option. #405

Merged
merged 3 commits into from
Aug 16, 2013

Conversation

xaviershay
Copy link
Member

Shows a non-trivial performance impact, though low absolute time for the common case.

I'm inclined to leave the default value as false, reconsidering if it happens to cause a lot of user confusion.

WDYT @myronmarston?

Resolves #395.

Shows a non-trivial performance impact.
@xaviershay
Copy link
Member Author

cc @pat as an FYI because you were involved in the original discussion.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0%) when pulling 8baa9b8 on xaviershay:issue-395 into c8139a9 on rspec:master.

@myronmarston
Copy link
Member

What do you think about adding a config option that specifies a default for transfer_nested_constants?

@pat
Copy link
Contributor

pat commented Aug 13, 2013

@xaviershay appreciate the cc, though you both know this topic far better than I do, so I'm happy to defer to your judgement.

@JonRowe
Copy link
Member

JonRowe commented Aug 13, 2013

I think given the performance difference its worth defaulting as false and then adding a RSpec.configuration option for those who feel otherwise.

@xaviershay
Copy link
Member Author

Added configuration option.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0%) when pulling 8140de9 on xaviershay:issue-395 into c8139a9 on rspec:master.

ensure
config.transfer_nested_constants = original_value
end
end
Copy link
Member

Choose a reason for hiding this comment

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

You can use the "with isolated configuration" shared context to manage the configuration isolation. Just include the context in your example group:

describe "the example group" do
  include_context "with isolated configuration"

  # ...
end

@myronmarston
Copy link
Member

This looks great. Nice and simple. Just one suggestion that will cleanup the spec a bit.

@xaviershay
Copy link
Member Author

@myronmarston oh neat. I added a small refactoring of the verifying double specs as well to use it too.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0%) when pulling 26dbe69 on xaviershay:issue-395 into 80d80d9 on rspec:master.

@myronmarston
Copy link
Member

This looks good. Nice to see that you were able to cleanup some other specs, too :).

I just kicked the errored builds (I suspect github's DDOS problems today are causing build issues).

@xaviershay
Copy link
Member Author

(it's green now too.)

myronmarston added a commit that referenced this pull request Aug 16, 2013
Benchmark of transfer_nested_constants option.
@myronmarston myronmarston merged commit db17429 into rspec:master Aug 16, 2013
@myronmarston
Copy link
Member

Maybe I've been doing it for too long, but it doesn't bother me that much. It is unambiguous, in that there aren't any other concept it could be referencing. You'd likely have to look up the documentation anyway, since it's not an obvious concept. Also, having it exactly match the option name is a nice property, even if it doesn't otherwise have the context. Two names for the same thing is possibly more confusing.

Works for me.

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.

Consider transferring nested constants by default when stubbing constants
5 participants