Skip to content

Conversation

@JoyceXu02
Copy link
Collaborator

@JoyceXu02 JoyceXu02 commented Mar 26, 2025

Description

Added some unitests for BrowserToolkit, and updated a safer url extractor.

Checklist

Go over all the following points, and put an x in all the boxes that apply.

  • I have read the CONTRIBUTION guide (required)
  • I have linked this PR to an issue using the Development section on the right sidebar or by adding Fixes #issue-number in the PR description (required)
  • I have checked if any dependencies need to be added or updated in pyproject.toml and uv lock
  • I have updated the tests accordingly (required for a bug fix or a new feature)
  • I have updated the documentation if needed:
  • I have added examples if this is a new feature
    Partially fixed [Feature Request] Enhance BrowserToolkit #1752
    If you are unsure about any of these, don't hesitate to ask. We are here to help!

@JoyceXu02 JoyceXu02 requested a review from Wendong-Fan March 26, 2025 01:39
Copy link
Member

@Wendong-Fan Wendong-Fan left a comment

Choose a reason for hiding this comment

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

thanks @JoyceXu02 's contribution! We’ve merged a PR that also updates the unit tests and example code. Could you review the current implementation and integrate your work with the existing one? Sorry for the inconvenience

@JoyceXu02 JoyceXu02 force-pushed the feat/enhance_browsertoolkit branch from d0fc548 to f4f16bb Compare March 28, 2025 00:18
@JoyceXu02 JoyceXu02 requested a review from Wendong-Fan March 29, 2025 15:40
Copy link
Member

@Wendong-Fan Wendong-Fan left a comment

Choose a reason for hiding this comment

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

thanks @JoyceXu02 , left comments below, also please pay attention to pre-commit check in cicd, now there's something need to be fixed

Comment on lines 550 to 552
# Use urlparser for a safer extraction the url name
parsed_url = urllib.parse.urlparse(self.page_url)
url_name = os.path.basename(str(parsed_url)) or "index"
Copy link
Member

Choose a reason for hiding this comment

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

seems urllib.parse.urlparse() returns a ParseResult object, and calling str(parsed_url) will convert the entire object to a string representation, not just the path componen, this will likely not give the expected result?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @Wendong-Fan, I updated url_name assignment.

@Wendong-Fan Wendong-Fan added the enhancement New feature or request label Mar 30, 2025
@Wendong-Fan Wendong-Fan added this to the Sprint 26 milestone Mar 30, 2025
@JoyceXu02
Copy link
Collaborator Author

thanks @JoyceXu02 , left comments below, also please pay attention to pre-commit check in cicd, now there's something need to be fixed

thanks @Wendong-Fan, pre-commit check has passed on my side.

Copy link
Member

@Wendong-Fan Wendong-Fan left a comment

Choose a reason for hiding this comment

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

thanks @JoyceXu02 for the contribution!

@Wendong-Fan Wendong-Fan merged commit 33b8bc2 into master Apr 9, 2025
6 of 7 checks passed
@Wendong-Fan Wendong-Fan deleted the feat/enhance_browsertoolkit branch April 9, 2025 04:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

[Feature Request] Enhance BrowserToolkit

3 participants