Defering to rspec-core when available without depending on it
#7
Conversation
|
I'm of the opinion that we need to move anything that's going to be shared between gems into -support, all the deprecation stuff falls under that definition, as such I'd recommend putting as much of the API we can over here. Adding extensions to what this gem does in the various gems if necessary. |
|
A few options:
I'm leaning towards 1 right now. 2 is very similar to what we have now, just refines the dependency. I still don't like the conditional. 3 is probably "most correct", but maybe too much overhead. |
That sounds good, but it's unclear from what you said if you favor putting the versions of those methods that depend on rspec-core here. |
|
@xaviershay -- the issue isn't RSpec.configuration. The issue is rspec-core's deprecation formatter (which is available off of |
|
@myronmarston I think that this gem should have no dependencies on any of the other rspec gems. So no, we shouldn't depend on RSpec core here. |
|
I also think rspec-support should not have dependencies on other rspec gems. |
|
@xaviershay also note that |
|
I'm going to retract 2 per @JonRowe's comment, and leave 1 and 3 for consideration. I don't understand the problem enough yet to argue for or against them :) Helicopter, out! |
|
What about if we rework this so it uses a 'deprecation output' and in RSpec Core we set this to the formatter, but support has a built in dumb one... |
Maybe. It sounds overly complex to me. What about my proposed solutions (e.g. having rspec-core override the definitions of these methods) do you find objectionable? |
|
Duplication and splitting of concerns... |
|
What duplication? You mean the method names? |
|
The fact that the knowledge of how we issue deprecations becomes split into two places. Sure it'd be two places rather than 3 as it is now but still.... |
|
I don't see duplication in that at all. I see a common interface (e.g. |
|
I see two definitions of the same interface, which could just as easily split as before. You're worried about circular dependance, which I don't really think it is (It's just a separate reverse dependancy to me, with 1 thing that can change) where as this creates a split dependency with both sides having to change simultaneously if it needs to change. So I would rather work on support owning this entirely... It's worth noting I don't really like the current deprecation formatter, I feel it hides information... |
I'm not sure what "separate reverse dependency" means. Never heard the term. And what's the 1 thing that can change?
Well, it depends entirely on what changes. Let me lay out how I see this. With your solution (keeping the rspec-core version of With my proposed solution (keeping the rspec-core version of I believe there is much more value in minimizing the coordination effort of changing the deprecation formatter than minimizing the coordination effort of changing the
I'm not sure what you mean by "it hides information", but we can definitely make it better. It'll be much harder to make it better once rspec-support depends on it, though. |
|
FWIW the current deprecation formatter was a feature request to reduce the sheer quantity of deprecation messages that can spew out when upgrading rspec on older projects. Presumably you don't have to see the same message repeated a thousand times to get the point. You -can- get a full deprecation report if you point the output to a file. Anyway, I think it eases the transition to newer versions of rspec, which is a good thing. I was initially against the idea myself, thinking the full output being painful was a feature, but I was eventually convinced (and implemented this version). The discussion was in rspec/rspec-mocks/issues/386 If there's an alternate version that would address the original issue, I'm open to refining this. |
|
I'm definitely a fan of what we wound up with, too. |
|
My concern with it is that when you have a high level of differing deprecations you end up obscuring the output of the tests, and when you have a high level of identical deprecations (as is the case with say, rspec-mocks and should) you don't get an indication of the scale of the problem. The discussion about this happened whilst both I and @samphippen were offline and I know he's voiced similar concerns to myself. But that's off topic. I'm going to think about this some more. |
|
As far as the scale of the problem, it does report As for the "differing deprecations", I do think this could be improved, but at the very least it now obscures the test output less because it's not interspersed within the test output. In one of my legacy applications the difference is 1066 lines interspersed the old way, and the more-recent deprecation formatter prints 52 lines at the end of a test run (with mixed types of deprecations - some identical and some differing). The latter is far more readable for the deprecation warnings and for the test results. |
I agree completely. |
Only define deprecation methods if they don't exist
|
Ok I'm deferring to everyone else here and just not defining the methods if they already exist. I'll update rspec/rspec-core#1115 accordingly |
Defering to `rspec-core` when available without depending on it
|
Thanks @JonRowe. There's another reason this change is important that @alindeman discovered: when running the rspec-rails cukes, he was getting lots of failures like: Cucumber doesn't load rspec-core but rspec-support was still defining the methods that depend on it! I think the reason this was happening is because our use of Andy found that applying this PR fixed his issue. |
Quoting @myronmarston :
My own immediate thoughts are that I'd rather have all this code in
-supportbut I can see @myronmarston's concerns. Originally the definition of this didn't depend on a formatter, and even if we were to only define the two deprecation methods if Core wasn't loaded we're splitting the responsibility for this functionality into two places. Which I don't like, however it would still eliminate 2 places (-mocksand-expectations) which is still a benefit. So...Thoughts?
/cc @samphippen @alindeman @soulcutter @xaviershay