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

[py] property strict_timestamp was added #14168

Merged
merged 7 commits into from
Jun 22, 2024

Conversation

iampopovich
Copy link
Contributor

@iampopovich iampopovich commented Jun 21, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

according to #14143 property was added to configure timestamp mode

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

Enhancement, Bug fix


Description

  • Introduced a new _strict_timestamps attribute to the FirefoxProfile class to manage timestamp behavior.
  • Added getter and setter methods for the strict_timestamps property.
  • Modified the encoded method to utilize the strict_timestamps property when creating a ZipFile.

Changes walkthrough 📝

Relevant files
Enhancement
firefox_profile.py
Add strict_timestamps property to FirefoxProfile class     

py/selenium/webdriver/firefox/firefox_profile.py

  • Added _strict_timestamps attribute with getter and setter methods.
  • Integrated strict_timestamps property into ZipFile instantiation.
  • +9/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 2
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    The new strict_timestamps property is set to True by default. Ensure this default setting aligns with the existing behavior or expectations of users to avoid breaking changes.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add a validation check in the setter to ensure the value is a boolean

    Consider adding a validation check in the strict_timestamps setter to ensure that the
    value being set is a boolean. This will help prevent potential issues if a non-boolean
    value is assigned.

    py/selenium/webdriver/firefox/firefox_profile.py [101-103]

     @strict_timestamps.setter
     def strict_timestamps(self, value: bool):
    +    if not isinstance(value, bool):
    +        raise ValueError("strict_timestamps must be a boolean")
         self._strict_timestamps = value
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding a validation check for boolean type in the setter method for strict_timestamps is crucial to prevent runtime errors and ensure type safety, which is a significant improvement.

    8
    Enhancement
    Rename the attribute to make it more descriptive

    To improve readability and maintainability, consider renaming the _strict_timestamps
    attribute to _strict_timestamps_enabled to make it more descriptive.

    py/selenium/webdriver/firefox/firefox_profile.py [56]

    -self._strict_timestamps = True
    +self._strict_timestamps_enabled = True
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Renaming _strict_timestamps to _strict_timestamps_enabled enhances readability and clarity of the code, making it easier to understand the purpose of the attribute.

    6
    Update property methods to use the new attribute name

    To ensure consistency, update the strict_timestamps property methods to use the new
    attribute name _strict_timestamps_enabled.

    py/selenium/webdriver/firefox/firefox_profile.py [98-103]

     def strict_timestamps(self) -> bool:
    -    return self._strict_timestamps
    +    return self._strict_timestamps_enabled
     
     @strict_timestamps.setter
     def strict_timestamps(self, value: bool):
    -    self._strict_timestamps = value
    +    self._strict_timestamps_enabled = value
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: If the attribute name is changed, it is essential to update all references to this attribute to maintain consistency. This suggestion correctly follows the change proposed in suggestion 2.

    6
    Maintainability
    Add a comment to explain the purpose of the new property

    Add a brief explanation or comment above the strict_timestamps property to clarify its
    purpose and usage, especially since it is a new addition.

    py/selenium/webdriver/firefox/firefox_profile.py [97-99]

    +# Property to get or set the strict timestamps for the zipfile
     @property
     def strict_timestamps(self) -> bool:
         return self._strict_timestamps
     
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: Adding a comment to explain the new strict_timestamps property improves code maintainability by making the codebase more understandable, although it's a relatively minor enhancement.

    5

    Copy link
    Member

    @titusfortner titusfortner left a comment

    Choose a reason for hiding this comment

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

    Thanks for adding this

    py/selenium/webdriver/firefox/firefox_profile.py Outdated Show resolved Hide resolved
    py/selenium/webdriver/firefox/firefox_profile.py Outdated Show resolved Hide resolved
    Copy link

    codecov bot commented Jun 22, 2024

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 57.18%. Comparing base (84828cd) to head (24b92ea).

    Current head 24b92ea differs from pull request most recent head fb52580

    Please upload reports for the commit fb52580 to get more accurate results.

    Additional details and impacted files
    @@           Coverage Diff           @@
    ##            trunk   #14168   +/-   ##
    =======================================
      Coverage   57.18%   57.18%           
    =======================================
      Files          89       89           
      Lines        5514     5514           
      Branches      232      232           
    =======================================
      Hits         3153     3153           
      Misses       2129     2129           
      Partials      232      232           

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    @milahu
    Copy link

    milahu commented Jun 22, 2024

    ok. squash commits as something like
    set strict_timestamps=False in zipfile.ZipFile
    or
    disable strict timestamps in ZipFile

    @titusfortner
    Copy link
    Member

    We squash as part of the GitHub merge process. Hmm, I'm not sure why that chrome test is failing in this PR but not on trunk, but at least it is obviously unrelated.

    @titusfortner titusfortner merged commit cd96b62 into SeleniumHQ:trunk Jun 22, 2024
    13 of 14 checks passed
    @titusfortner
    Copy link
    Member

    Thanks @milahu & @iampopovich; sorry that I made a one-line change more difficult than it needed to be.

    @iampopovich iampopovich deleted the 14143-fix-ff-profile branch June 23, 2024 03:10
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Oct 29, 2024
    * allow zipping profiles with files and directories stamped earlier than 1980
    
    ---------
    
    Co-authored-by: Titus Fortner <[email protected]>
    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