Added possibility to combine index and filename args#426
Conversation
If test are run, to get a custom and unique filename each time screenshot is takes, it would require creating blobal variable in RF data and making sure that variable in unique for each time screenshot is taken. This is now much easier to do with the index param in the Capture Page Screenshot keyword.
|
And did forget to say that removed the |
|
This should probably be the default behavior. @pekkaklarck, @ombre42 If we take a screen shot and specify the file to save it to but that file already exists, should we append a |
documentation.
|
@zephraph Perhaps if you use Wait Until Keyword Succeeds? But that is quite a long shot, in my mind. |
There was a problem hiding this comment.
Change index here to overwrite and default it to False.
|
@aaltat, I've made suggestions on how I think it should be. I'd like it to always append the index unless it's specified not to. Also, Instead of having the argument |
|
I'd say that, in general, if you give an exact file and that file exist, it should be overwritten. Not certain about this particular case, though. Having an option to control the behavior is obviously the most generic solution. |
|
I suppose the default doesn't really matter to me so long as its well documented. |
|
FWIW, `overwrite=False' sounds good to me as the default. I don't feel too strongly about this either way. |
|
I feel also that false is better, I will try to do changes today or tomorrow. |
Now if the filename exist, new one is created by adding counter in the end (but before .png). All the existing acceptance tests do pass and where not modified. New ones where modified to meet the new requirements.
|
Changed code to more clearly to do what @zephraph expressed in the keyword documentation comment. And now there is a global and common counter for all files and I also do not like it. But I was thinking that could be separate pull request and perhaps add more formatting to the counter? Not like 1,2,3 but like 001, 002 and so on. |
|
And now the acceptance test are, for this new feature, in one blob/test which is not good. Should I refactor that be more clear? |
|
I'm still not 100% satisfied with this, but you're definitely on the right track. I'm going to submit a PR to your branch with what I had in mind. |
Now for each filename, there is unique index saved in a dictionary. This makes the end result, the screenshots, more easier to read and maintain.
|
Refactored the code to contain unique index for each file name. Also changed to acceptance test be more readable. Is this OK? This is how to code now works. |
|
I'll review this after work today. At first glance though it looks pretty good. |
|
Noticed that I did forget to update the keyword documentation. I am busy for next few days, but I will do it next week. |
|
That's fine, the next minor release won't be until at least the end of next week. |
|
The keyword documentation is now fixed, at least for my eyes. Comments? |
|
This looks pretty good. It's varies a little from my original intent, but I think it's ample for this problem. Give me a little while to review it. If I don't find anything I'll merge it in later this afternoon. Thanks for all your hard work @aaltat. |
|
How does it looks and should I do something because master has moved forward? |
|
Oh yeah, this'll be interesting... I've done a bit of work on screenshots recently so there will be a bit to merge. |
I was looking at this file and noticed an unused import.
Another unused import here.
|
Yeah, let's just close this in favor of #465 |
4 participants
If test are run, to get a custom and unique file name each time screenshot is takes, it would require creating global variable in RF data and making sure that variable in unique for each time screenshot is taken. This is now much easier to do with the index param in the Capture Page Screenshot keyword.
This comes quite handy when you using pabot and want to make sure that screenshots do not overlap in names.