Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Imporve the indexing function for the pages screenshot. #1840

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

AmirHdm
Copy link

@AmirHdm AmirHdm commented Jun 15, 2023

Add indexing functionality based on datetime for capture screen page method inside ScreenshotKeywords class.

…_get_screenshot_path

/*
def _get_screenshot_path(self, filename):
        if self._screenshot_root_directory != EMBED:
            directory = self._screenshot_root_directory or self.log_dir
        else:
            directory = self.log_dir
        filename = filename.replace("/", os.sep)
        YY_MM_DD_HH_MM_SS = datetime.datetime.now().strftime('%Y-%m-%d_%H-%M-%S')
        while True:
            YY_MM_DD_HH_MM_SS = datetime.datetime.now().strftime('%Y-%m-%d_%H-%M-%S')
            formatted = _format_path(filename, YY_MM_DD_HH_MM_SS)
            path = os.path.join(directory, formatted)
            # filename didn't contain {YY_MM_DD_HH_MM_SS} or unique path was found
            if formatted == filename or not os.path.exists(path):
                return path
*/

This action is inprogress
Selenium-Screenshot-{index}
index : 'YYYY-MM-DD-HH-MM-SS-RANDOM
@emanlove
Copy link
Member

@AmirHdm Just wanted to let you know that your pull request has been noticed and within the queue to be reviewed. My plan was to bring this to the team to review and discuss. I will update you sometime within the next couple weeks.

@AmirHdm
Copy link
Author

AmirHdm commented Jun 23, 2023

@AmirHdm Just wanted to let you know that your pull request has been noticed and within the queue to be reviewed. My plan was to bring this to the team to review and discuss. I will update you sometime within the next couple weeks.

@emanlove great ! And thanks for the feedback

@lisacrispin
Copy link

@AmirHdm Is this changing the naming of screenshot files from simply numbering them sequentially based on the index, to using the timestamp in the name? That does seem like a good idea, if so.

@AmirHdm
Copy link
Author

AmirHdm commented Jun 23, 2023

@lisacrispin yeath that's right because using timestamps in naming is more helpful and significantly

@emanlove
Copy link
Member

@AmirHdm I like the idea of providing different type of "indexing" or filenames. One concern I do have though is breaking any existing systems that rely on numerical indexing. Relooking at the code and keyword documentation I see for singular Capture Page Screenshot there is a filename argument which allows one to format the filename. I'm curious as to whether we could use this "syntactical idea" to allow for something like filename = selenium-screenshot-{timestamp}.png or filename = selenium-screenshot-{timestamp:%Y-%m-%d-%H-%M-%S}.png or maybe even filename = selenium-screenshot-{random}.png and apply it to the default capture screenshot on error functionality?

@AmirHdm
Copy link
Author

AmirHdm commented Jun 24, 2023

@AmirHdm I like the idea of providing different type of "indexing" or filenames. One concern I do have though is breaking any existing systems that rely on numerical indexing. Relooking at the code and keyword documentation I see for singular Capture Page Screenshot there is a filename argument which allows one to format the filename. I'm curious as to whether we could use this "syntactical idea" to allow for something like filename = selenium-screenshot-{timestamp}.png or filename = selenium-screenshot-{timestamp:%Y-%m-%d-%H-%M-%S}.png or maybe even filename = selenium-screenshot-{random}.png and apply it to the default capture screenshot on error functionality?

@emanlove sure i guess we could apply your idea and actually it's a great feature then let me see what i could do and will give a quick feedback

…ature

Add formatting functions for a randon & timestamp  placeholders
… {random} or {timestamp}.png'

Add three indexing option : index / random / timestamp
@AmirHdm
Copy link
Author

AmirHdm commented Jul 4, 2023

The user now is able to set 3 indexing options on the keyword capture page screenshot :

  • filename: str = 'selenium-screenshot-{index}.png'
  • filename: str = 'selenium-screenshot-{random}.png'
  • filename: str = 'selenium-screenshot-{timestamp}.png'

# filename didn't contain {index} or unique path was found
if formatted == filename or not os.path.exists(path):
return path

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick read of this I believe there is a logic error here where if none of the three are in the filename one will get themselves into an infinite loop.

@emanlove emanlove added this to the v6.2.0 milestone Sep 26, 2023
@emanlove emanlove modified the milestones: v6.2.0, v6.3.0 Nov 19, 2023
@emanlove emanlove modified the milestones: v6.3.0, April 2024 Mar 7, 2024
@emanlove emanlove modified the milestones: April 2024, May 2025 Apr 20, 2024
@emanlove emanlove modified the milestones: May 2024, June 2024 Jun 10, 2024
@emanlove emanlove modified the milestones: Sept 2024, Oct 2024 Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants