Skip to content

Conversation

WSUFan
Copy link
Contributor

@WSUFan WSUFan commented Aug 8, 2024

This PR addresses a typo and improves the handling of absolute URLs in the
parse_simpleapi_html.bzl file and corrects a minor issue in the
simpleapi_download.bzl file.

Summary:

  1. parse_simpleapi_html.bzl:

    • Introduced a new private function _get_root_directory(url) to extract
      the root directory from a given URL.
    • Enhanced _absolute_url function to correctly handle absolute URLs by
      utilizing the _get_root_directory function. This ensures that URLs
      starting with a "/" are correctly resolved to their full path, avoiding
      potential incorrect concatenation.
  2. simpleapi_download.bzl: Corrected the handling of the real_url variable in
    the _read_simpleapi function, ensuring that the correct URL is passed to
    _read_index_result when using non-blocking downloads.

@WSUFan WSUFan requested review from aignas and groodt as code owners August 8, 2024 16:47
Copy link

google-cla bot commented Aug 8, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@WSUFan WSUFan force-pushed the fix/typo-and-url-processing branch 2 times, most recently from 6dc34e1 to ce43173 Compare August 8, 2024 17:00
@aignas
Copy link
Collaborator

aignas commented Aug 11, 2024

Could you please add a test in https://github.com/bazelbuild/rules_python/blob/3b183bfedd3dd6a5bb42c77531079e347165441d/tests/pypi/parse_simpleapi_html/parse_simpleapi_html_tests.bzl so that this does not regress?

And please add a note in the Changelog.md file.

@WSUFan
Copy link
Contributor Author

WSUFan commented Aug 14, 2024

Could you please add a test in https://github.com/bazelbuild/rules_python/blob/3b183bfedd3dd6a5bb42c77531079e347165441d/tests/pypi/parse_simpleapi_html/parse_simpleapi_html_tests.bzl so that this does not regress?

And please add a note in the Changelog.md file.

Yeah. Sure. I will do it. Thanks!

@WSUFan WSUFan force-pushed the fix/typo-and-url-processing branch from ce43173 to c625eac Compare August 14, 2024 21:14
@WSUFan WSUFan requested a review from rickeylev as a code owner August 14, 2024 21:14
- Add _get_root_directory(url) function to extract the root directory from a given URL.
- Update _absolute_url function to handle absolute URLs by concatenating them with the root directory.
- Fix a typo in simpleapi_download.bzl to ensure real_url is passed correctly to _read_index_result in non-blocking download scenarios.

These changes improve the reliability of URL processing and download handling in PyPI-related scripts.
@WSUFan WSUFan force-pushed the fix/typo-and-url-processing branch from c625eac to a8a8db1 Compare August 14, 2024 21:15
@WSUFan
Copy link
Contributor Author

WSUFan commented Aug 14, 2024

Could you please add a test in https://github.com/bazelbuild/rules_python/blob/3b183bfedd3dd6a5bb42c77531079e347165441d/tests/pypi/parse_simpleapi_html/parse_simpleapi_html_tests.bzl so that this does not regress?

And please add a note in the Changelog.md file.

Hi @aignas, I have added tests and changed the Changelog.md. Please take a look when you get a chance. Let me know if you have other concerns. Thanks!

@WSUFan WSUFan requested a review from aignas August 14, 2024 21:17
Copy link
Collaborator

@aignas aignas left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.

The current structure is alright, we can optimize the code if we see any problems later. At the end of the day, when using the MODULE.bazel.lock file, this code will be executed only when the PyPI dependencies change and the time spent querying the PyPI server itself is greater than the time spent parsing it.

Thank you very much for the contribution!

@aignas aignas added this pull request to the merge queue Aug 15, 2024
Merged via the queue into bazel-contrib:main with commit d69559d Aug 15, 2024
@WSUFan WSUFan deleted the fix/typo-and-url-processing branch August 15, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants