Using Capture Screenshot with %d in filename#504
Conversation
|
👍 |
There was a problem hiding this comment.
Why would an exception be raised because of there being an extra % in the name?
There was a problem hiding this comment.
It is because of Python older string formatting, which the code uses, see in here: https://github.com/robotframework/Selenium2Library/pull/504/files#diff-5346a546c6b37e3c016ad51e7155887dR122. Example this: python -c "'foo % bar %d' % 'string'" will also cause exception. There is bug in documentation, that exception is not always TypeError and it would be best to say only exception.
The Python 2.6 onward supports str.format(), which could solve the problem in different way but because we must support Python 2.5, I did not use it. I could also do simple str.replace() in the code, but I would like to keep some formatting options open for future enhancements.
|
I initially proposed because you could just use Because I think other formatting isn't likely very useful, I don't think we need to take that into account. That means we could easily use something more explicit than |
|
Also, @aaltat mentioned that |
|
It is not promised directly. But we do claim to support quite old versions of Robot Framework (2.6 is the minimum version) and older versions do support Python 2.5. How realistic this is, ido not know. But if we announced that we only support 2.6 onwards, it would allow to use |
|
As I already wrote, Selenium itself hasn't supported Py2.5 for years. Do you think someone would be using new S2L and downgrade Se separately to an ancient version? Supporting formatting like leading zeros is a very good point. Supporting |
|
Ah my mistake, in INSTALL.rst we say that 2.6 is the minimum version. Because of the trouble caused by |
|
Refactored the pull request to use |
There was a problem hiding this comment.
This note should be more explicit about what's new in 1.8. I would also either move it at the end of the doc or embed it into the doc like Starting from version 1.8, if ....
There was a problem hiding this comment.
Went for Starting from version 1.8, if ...
|
@pekkaklarck Thanks for good review. |
|
I just read through it and everything looks good to me. You've got my 👍 to merge whenever you feel this is ready. |
|
If @pekkaklarck is also happy, then I am also ready for merge |
|
Squashed all to commits to one, because the middle commits where not meaningful alone. |
|
Hmm some other test did fail, can I somehow push Travis to rerun the tests? |
There was a problem hiding this comment.
Are these changes here to avoid lines longer than 80 chars? That is OK in general, but such changes should be submitted separately. Preferably so that the whole module is gone through in one go. In minor cases it's also a bit questionable is the slightly enhanced PEP-8 compatibility worth an extra change in the version history.
There was a problem hiding this comment.
I mostly agree with what @pekkaklarck said. We really should keep changes focused. With that being said, I'm not necessarily against this staying. I'll defer to you two on that decision.
There was a problem hiding this comment.
Yes, modifications where done do void too long lines. I can can remove it from the commit if it's needed.
There was a problem hiding this comment.
No need to remove. Unrelated changes just add noise that makes the review a bit harder. Now we all already know why the changes are here and removing them wouldn't win much.
|
Sure thing. |
|
I did fix it based on the comments from @pekkaklarck. @zephraph could you do review also. |
There was a problem hiding this comment.
There's a typo here. Plus it's worded a little weird. Maybe it should be something like
Capturing a page screenshot with two indexes should not cause an error
Or something, idk. Regardless, it could definitely be reworded.
There was a problem hiding this comment.
True, wording is not correct and your example is better. But should I remove the hole test and what you mean: worked weird?
|
@aaltat, have you ran this locally to make sure that the screenshots really do show up? |
|
I did manualy verify that screen captures are visible in the log. |
|
Found two more small typos. Otherwise, it looks good to me. Fix those and I'll merge it in. Thanks for your patience! |
This commit enhances the Capture Screenshot keyword to support
internal running counter also when filename is not None. The place
of the counter is indicated with {index} characters.
Also the absolute path of the screenshot filename is always returned
by the keyword.
|
Fixed based on @zephraph comments. I think, this is normal iterative development process. I did have an idea, or actually problem to solve. Made a first attempt to fix it and honesty it was quite horrible attempt. Then there was new idea from the review process, which was better, but caused lot of problems/corner cases. And on last attempt, we did nail it down quite well. Also I am used to review process, it is nice way to share the information between people. It also yields to better results (and this is fine example of one), like better functionality, cleaner code and so one. |
|
Agreed. |
|
👍 |
Use Capture Screenshot with {index} in filename.
3 participants
This commit enhances the Capture Screenshot keyword to support
internal running counter also when filename is not None. The place
of the counter is indicated with %d character.
Also the absolute path of the screenshot filename is always returned
by the keyword.
Also made small drive by refactoring to follow pep8 better.