Screenshot fixes #986
Screenshot fixes #986
Conversation
This was a regression caused by new architecture.
In practice removed the support to automatically restore the old configuration. Instead the old value is returned similarly as with other "Set ..." keywords. Fixes robotframework#985.
| "keyword is deprecated and has no effect.") | ||
| previous = self.ctx.screenshot_root_directory | ||
| self.ctx.screenshot_root_directory = path | ||
| return previous | ||
|
|
||
| def _create_directory(self, path): |
There was a problem hiding this comment.
The method is used also in capture_page_screenshot method. It would be cleaner to move the _create_director method after the capture_page_screenshot methods.
There was a problem hiding this comment.
This is highly subjective. There's no rule to dictate how to order (helper) methods that would work well in all cases. A common approach, and also approach I generally prefer, is to have helper methods right after their first usage. I don't feel strongly about it here, though.
There was a problem hiding this comment.
It's highly subjective. The general style, before my time, with the library has been the helpers methods go after the keyword methods. Should we change the style?
There was a problem hiding this comment.
Having all helper methods at the end in somewhat random order is, in my opinion, a bad practice. I like the stepdown rule recommended in the Clean Code book by Robert C. Martin: https://stackoverflow.com/questions/21881605/resharper-and-stylecop-vs-the-step-down-rule-clean-code
Even with that style there's a question how to handle same helper used multiple times. I prefer having the helper after the first usage, but here it's recommended to have it after the last: http://tidyjava.com/stepdown-rule/
Anyway, I don't think it is a good idea to do larger changes to the code just to change the style, but I'd be in favor of moving towards the stepdown rule. Perhaps having helpers after the last usage would be best, because it would be closer to the current approach.
Notice also that lot of this problem goes away when classes aren't too big. Some of the internal classes could still be split to smaller pieces.
See messages of the individual commits for details.