Proposal cleaning #247
Proposal cleaning #247
Conversation
| def self.run | ||
| new.run | ||
|
|
||
| # values used in defined functions |
There was a problem hiding this comment.
Looks like all this initializations ended in the wrong place after code shuffling (self.run instead of initialize)
|
As always when refactoring without tests, I'm quite afraid of slippering bugs in. In general I like the reorganization into ProposalRunner and ProposalStore. Looks much better than before, even if I lack a clear full picture to give a good judgement. |
|
@ancorgs I plan to do manual and also write automatic tests, I just want at first discuss design, as it can change whole structure and require test rewrite. |
|
added some fixes after manual testing, automatic tests will follow |
|
|
||
| it "runs" do | ||
| # TODO: just catch exceptions | ||
| expect(subject.run).to eq :abort |
There was a problem hiding this comment.
Huh, why should a plain run return :abort? I would expect :next.
What do you mean by "just catch exceptions"? Is it "the code does whatever, but does not raise an exception?".
There was a problem hiding this comment.
yes, it is trivial test that there is no syntax or nil error. :abort because some parts are not set and it quit quickly.
There was a problem hiding this comment.
So "runs" is obviously not a enlightening description.
There was a problem hiding this comment.
ok, maybe "not crash" is better. Runner is quite tricky and not easy to test, so I just add basic test that there is no trivial error.
There was a problem hiding this comment.
Yes, it is perfectly fine to have such a test, only it should be phrased slightly differently IMHO:
it "passes a smoke test" do
expect { subject.run }.to_not raise_error
endA "smoke test" is a term from electronics: you solder a device, power it on, and nothing happens. No smoke comes out :)
There was a problem hiding this comment.
@mvidner good idea, I will change it this way.
|
@mvidner your suggestion implemented |
|
|
||
|
|
||
| tabs = properties["proposal_tabs"] | ||
| @tab_labels = tabs.map { |m| m["label"] || "Tab" } |
There was a problem hiding this comment.
That element is compulsory, remove || "Tab"
|
Overall, this is a partial clean-up of a big piece of code. Therefore, it is important to know what is in scope. For me, it was describing the shorter one of the new classes, |
|
@mvidner I fixed most of issues, except ones I comment. Maybe next time regarding documentation it will be faster if you directly commit to branch |
|
Thank you!
I see you have made a nice conflict with your own Rubocop changes... hope most can be autocorrected. |
|
LGTM now :) |
|
@mvidner thanks for very exhausting review 👍 |
not yet ready for merge, need real testing and also automatic tests. More for discussion of design.