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

Run specs for warning helpers in sub process #6

Merged
merged 1 commit into from
Oct 18, 2013
Merged

Conversation

JonRowe
Copy link
Member

@JonRowe JonRowe commented Oct 18, 2013

No description provided.

end
def run_with_rspec_core
in_sub_process do
require 'rspec/support/warnings'
Copy link
Member

Choose a reason for hiding this comment

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

This require concerns me, because the spec suite for this gem could grow to the point where this file is required elsewhere, and this would be come a no-op. Have you considered using load instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed.

@myronmarston
Copy link
Member

I'm worried that we're creating a circular dependency here: rspec-core depends on this gem, and this gem now implicitly depends on rspec-core, and that could create a maintenance headache. (Consider, for example, what happens when we decide to change the deprecation formatter API: we'll have to coordinate changes in rspec-core and rspec-support).

Do you think there's a reasonable way to get this to work that doesn't involve creating that circular dependency?

For example, maybe this gem could just define the core-less version of these methods, and then core can override those definitions. Or here it could define the methods only if defined?(::RSpec::Core) is false, and then rspec-core can take care of defining its versions.

Thoughts?

JonRowe added a commit that referenced this pull request Oct 18, 2013
Run specs for warning helpers in sub process
@JonRowe JonRowe merged commit 610e7d7 into master Oct 18, 2013
@JonRowe JonRowe deleted the run_in_subprocess branch October 18, 2013 11:44
@JonRowe
Copy link
Member Author

JonRowe commented Oct 18, 2013

(Note I'm merging this because it doesn't relate to your concerns directly, I'm going to pull those into a new issue)

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