Capture Page Screenshot should not overwrite if file already exist

[aaltat]

The #501 tries to fix the issue that if the file already exist, then index is added to the file name and file is created with index in file name. Example:

| Capture Page Screenshot | filename.png |
| Capture Page Screenshot | filename.png |
| File Should Exist | filename.png |
| File Should Exist | filename-1.png |

Making issue also, because it helps perform better tracking and if I screw the pull request again, we do not lose the conversation history.

[pekkaklarck]

Should this possibly be configurable with an optional parameter to avoid backwards incompatibility issues? Or should we support something like file-%d.png that tells S2L to replace ℅d with an index? It would also allow configuring where the index is added.

Regardless is the index configurable, it would be a good idea for the keyword to return an absolute path to the created image.

[aaltat]

There is overwrite=False parameter to control indexing and therefore the change is not backwards compatible. If we should set it to overwrite=True then it would work like the old implementation. This was briefly discussed in #426 but I also feel that we did not think the issue close enough.

I agree that returning the path is a good idea and I will make the change.

The %d is and interesting proposal. It also opens door to other formatting rules which could be nice to have (but perhaps in next commit and not in this one). With fast thinking, it should not be too hard to do.

So should this be default:

| Capture Page Screenshot | filename.png | 
| Capture Page Screenshot | filename.png |
| File Should Exist | filename.png |
| File Should Not Exist | filename-1.png |

In case of %d, should we support something like this

| Capture Page Screenshot | filename-%d.png | 
| Capture Page Screenshot | filename-%d.png |
| File Should Exist | filename-1.png |
| File Should Exist | filename-2.png |

In this scenario, should we also keep the overwrite argument.

[pekkaklarck]

I don't think both ℅d and overwrite would be needed. Nor do I know which would be better solution...

[aaltat]

My need is purely to avoid overwrites when I run my test with pabot. I simply want do define the filename with variable l, like ${BROWSER} and I would like avoid adding unique runing number in the end in the robot data.

The %d opens nice possibilities and could be more useful. Perhaps I should write a prototype and we can see in which way is the best.

[pekkaklarck]

+1 for prototypes. After thinking this a bit more, I start to like %d approach more and more. Making overwrite the default behavior would be more convenient in some cases but being backwards incompatible in others is a big minus.

[aaltat]

I am also starting to think that %d is best way to go. Also returning the absolute path is a good idea. I will write prototype and let's see how it does look.

[aaltat]

Made the proposal to support %d in #504

#504 proposal does not check does the file exist, but it also meets my needs. Personally I think the #504 is better and provides easier way extend the functionality in the future if needed. The only, although small, possibility for backwards incompatible is that someone might be already use %d in their filename.

[just-be-dev]

I like this much, much better. I completely agree with both of you, the %d approach looks excellent.

[aaltat]

I am thinking the same way and let's go with the %d option. I will test few corner cases, read the code again and at least write better tests.

One thing which comes in my mind, that the code is written now in way that it does not count the number of %d in the filename. If there are more than one, it will raise an exception. But I will give it some thoughts and let's see...

[just-be-dev]

I don't think it would make sense to allow more than one. Check for multiples, raise an exception, and document it.

[aaltat]

True, I will document it and write a test.

[aaltat]

I did updated documentation and re-write the tests in pull request #504. Review would be really nice.

[aaltat]

Did refactor the %d to {index} in #504 because it was causing more problems that it tried to solve. Also now it uses str.format in the underlying code and it solves the problems that the %d did have.

I am not totally sure, did I get the documentation totally in the way that would be correct, staring from here: https://github.com/robotframework/Selenium2Library/pull/504/files#diff-5346a546c6b37e3c016ad51e7155887dR55

Comments...

[just-be-dev]

The content of the documentation itself looks good. Formatting can always be tricky, but nothing jumps out at me. You should probably generate out the documentation from your branch and make sure it looks right before merging.