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] document cygwin path for send_keys #15275

Merged
merged 3 commits into from
Feb 13, 2025
Merged

Conversation

Delta456
Copy link
Member

@Delta456 Delta456 commented Feb 11, 2025

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.

Motivation and Context

Adds extra doc for send_keys when using Cygwin. Related to #15272

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

Documentation


Description

  • Added documentation for handling file paths in Cygwin for send_keys.

  • Clarified usage of Windows-style paths for Cygwin users.

  • Updated example code in send_keys docstring.


Changes walkthrough 📝

Relevant files
Documentation
webelement.py
Update `send_keys` docstring with Cygwin-specific guidance

py/selenium/webdriver/remote/webelement.py

  • Added a note about using Windows-style paths in Cygwin.
  • Updated example code to demonstrate correct path formatting.
  • +2/-0     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    15272 - Partially compliant

    Compliant requirements:

    • Add documentation to clarify that Cygwin users need to use Windows-style paths in send_keys
    • Fix example code to show correct path handling for Cygwin users

    Non-compliant requirements:

    • Fix misleading advice about using os.path for cross OS testing - the original advice about using os.path is still present in the docstring
    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Misleading Documentation

    The docstring still recommends using os.path for cross OS testing, which contradicts the Cygwin-specific guidance and was part of the original issue

    >>> # Generally it's better to wrap the file path in one of the methods
    >>> # in os.path to return the actual path to support cross OS testing.
    >>> # file_input.send_keys(os.path.abspath("path/to/profilepic.gif"))

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix inconsistent path separators

    The Cygwin path conversion example has incorrect path separator handling - it
    replaces forward slashes with backslashes but the example shows forward slashes.
    This can cause path resolution issues.

    py/selenium/webdriver/remote/webelement.py [285-286]

     >>> # When using Cygwin, the path need to be provided in Windows format.
    ->>> # file_input.send_keys(f"C:/cygwin{os.path.abspath('path/to/profilepic.gif').replace('/', '\\')}")
    +>>> # file_input.send_keys(f"C:/cygwin{os.path.abspath('path/to/profilepic.gif')}")
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies a critical inconsistency in path handling that could cause file resolution failures - the example shows forward slashes but attempts to convert to backslashes, which would break the path format. The fix ensures consistent path separator usage.

    Medium

    @Delta456 Delta456 requested a review from VietND96 February 11, 2025 12:28
    @Delta456
    Copy link
    Member Author

    I feel this can be generalised and users can read this docstring easier because functions like send_keys would need the same 🤔

    @VietND96
    Copy link
    Member

    I read somewhere that mentioned when using this

    input_path = 'C:/Users/user/Downloads/file.docx'
    input_path = os.path.normpath(input_path)
    # On Windows: input_path = 'C:\Users\user\Downloads\file.docx'
    

    Do you think we can handle this for cross-platform instead of documenting only?

    @VietND96
    Copy link
    Member

    After discussing, we keep the update in documentation only.

    @VietND96 VietND96 merged commit 6836d0a into SeleniumHQ:trunk Feb 13, 2025
    16 of 17 checks passed
    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.

    2 participants