Refactor swt specs to use a shared context #497
Conversation
Also, eliminate temporary test-double shims for allowing differently-named doubles in shared specs
| @@ -13,7 +13,7 @@ def initialize(app, point_a, point_b, opts = {}) | |||
| @app = app | |||
|
|
|||
| @style = Shoes::Common::Stroke::DEFAULTS.merge(opts) | |||
| @style[:strokewidth] ||= @app.style[:strokewidth] || 1 | |||
| @style[:strokewidth] ||= 1 | |||
There was a problem hiding this comment.
Why did you get rid of this? Shouldn't @app.style be the right place to save the global styles?
There was a problem hiding this comment.
I got rid of this because it was the only style attribute being treated this way. The others are set either in the *::DEFAULTS hashes or by merging with the app's style hash in the DSL methods. I didn't think it made sense to treat :strokewidth differently. Plus, it required stubbing #style on the app double in the specs.
There was a problem hiding this comment.
Hmmm I personally don't like that this is merged in the DSL method as imo it's not related to the DSL method as parsing the DSL but is rather an important value... we could investigate that at another point though.
|
I'm a huge 👍 on this refactoring. A lot of the comments are just about deleting out commented talk and some questions (see code comments). As for rebasing/merge both are fine with me. Although it mgiht be nice to sqquash the separate parts of this PR together. E.g. redraw refactoring, shared context, specs failing on OSX Thank you Eric! 🚀 |
|
@PragTob thanks for the review. I responded to questions inline. Thanks for picking up on the commented-out code I forgot to delete! |
|
So is there anything missing before we can merge this in? =) |
|
Finally got down to giving it the read-through I'd been meaning to this morning. |
|
Waiting for a word from @wasnotrice about if he still wants to add anything or if we can go ahead, emrge this in and make all our SWT specs writing lives much better! =) |
|
Replaced by #507 (same content) |
This is a ginormous refactor of the SWT specs to use a shared context, as @jasonrclark suggested in #482. Specs are broken at almost every individual commit, so it might be best to squash this if we decide to merge it.
This is all a bunch of yak shaving, actually. I was trying to refactor some of the toggle code that looked like
@dsl.app.gui.real.disposed?so that we had some better encapsulation, but that was hard to do because of the tests. So you will see some of that along the way--attempts to follow the Law of Demeter. I also removed some unused references, particularly to@container.I think this will make it much easier to understand the specs, and to change the code without spuriously breaking lots of specs. Thoughts?