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

relative links_fix #134

Merged
merged 28 commits into from
Aug 3, 2023
Merged

Conversation

gmathiou4
Copy link
Contributor

This a fix for issue
#129

  • It adds the ability to handle relative links
  • first check if a link is relative and joining to the full package url

Signed-off-by: Georgios Mathioudakis [email protected]

@gmathiou4 gmathiou4 force-pushed the Relative-links-confuse-PyPi branch 2 times, most recently from 3513ff1 to 18f1b90 Compare June 15, 2023 14:14
@TG1999
Copy link
Contributor

TG1999 commented Jun 15, 2023

@gmathiou4 thanks for the PR, it would be nice if you could add some tests to verify your changes.

@gmathiou4 gmathiou4 force-pushed the Relative-links-confuse-PyPi branch 3 times, most recently from aed1540 to f5a3fc6 Compare June 16, 2023 15:13
@gmathiou4
Copy link
Contributor Author

gmathiou4 commented Jun 16, 2023

@TG1999

Still i have 2 failing tests unrelated to my changes

FAILED tests/test_codestyle.py::BaseTests::test_codestyle - Exception: ('Code style check failed!', b"-> Run pycodestyle (PEP8) validation\n./inspec/lib/python3.11/site-packages/pip_requirements_parser.py:103:77: W291 trailing whitespac...
FAILED tests/test_resolution.py::test_without_supported_wheels - AttributeError: 'IfExp' object has no attribute 'elts'

How would you suggest to proceed with those?

@gmathiou4 gmathiou4 force-pushed the Relative-links-confuse-PyPi branch 2 times, most recently from 52d1969 to e987814 Compare June 19, 2023 14:43
@TG1999
Copy link
Contributor

TG1999 commented Jun 20, 2023

@gmathiou4 for
FAILED tests/test_codestyle.py::BaseTests::test_codestyle - Exception: ('Code style check failed!', b"-> Run pycodestyle (PEP8) validation\n./inspec/lib/python3.11/site-packages/pip_requirements_parser.py:103:77: W291 trailing whitespac

Please run make valid command

@TG1999
Copy link
Contributor

TG1999 commented Jun 20, 2023

@gmathiou4 I ran the tests on your branch locally, I was not able to re-create this error
FAILED tests/test_resolution.py::test_without_supported_wheels - AttributeError: 'IfExp' object has no attribute 'elts'

@gmathiou4

This comment was marked as outdated.

@gmathiou4 gmathiou4 force-pushed the Relative-links-confuse-PyPi branch 6 times, most recently from 37e01f6 to e027c0d Compare June 26, 2023 16:51
@gmathiou4
Copy link
Contributor Author

@TG1999 The tests are fixed now, please proceed with the review

Copy link
Contributor

@TG1999 TG1999 left a comment

Choose a reason for hiding this comment

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

Please add a test for the package which have relative URLs to check the whole functionality

if "data-requires-python" in anchor_tag.attrs:
python_requires = anchor_tag.attrs["data-requires-python"]
# Check if the link is a relative URL
if not url.startswith(("http://", "https://")):
base_url = "/".join(package_url.split("/")[:-1]) # Extract base URL
Copy link
Contributor

Choose a reason for hiding this comment

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

Please extract this part of code in a separate function and add some doctest or unit tests for same.

Copy link
Contributor Author

@gmathiou4 gmathiou4 Jun 28, 2023

Choose a reason for hiding this comment

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

Hello @TG1999 , i have already added 2 new files for the test
tests/data/fetch_links_test.html
tests/data/relative-links-expected.json
and modified
tests/test_utils.py

Those tests aren't suitable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TG1999 Could you please provide a feedback, is the change ready for merge?

@gmathiou4 gmathiou4 force-pushed the Relative-links-confuse-PyPi branch 2 times, most recently from 2d1fe9e to db27f06 Compare June 28, 2023 11:52
links.append(Link(url=url, python_requires=python_requires))
# TODO: keep sha256
return links


def resolve_relative_url(package_url, url):
"""
Resolve a relative URL based on the package URL.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add some doctests

@TG1999
Copy link
Contributor

TG1999 commented Jul 3, 2023

@gmathiou4 thanks! just a minor nitpick for your consideration #134 (comment)

@gmathiou4 gmathiou4 force-pushed the Relative-links-confuse-PyPi branch 6 times, most recently from eb3c585 to 6f35786 Compare July 13, 2023 08:12
@gmathiou4
Copy link
Contributor Author

Hello,
Could you please review the changes and let me know if you have another suggestion before the merge?

@TG1999
Copy link
Contributor

TG1999 commented Aug 3, 2023

@gmathiou4 please squash your commits

Copy link
Contributor

@TG1999 TG1999 left a comment

Choose a reason for hiding this comment

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

LGTM!

@pombredanne
Copy link
Member

@TG1999 the tests are failing... may be a skeleton issue? I see also #119 ... so I guess I'll approve and we can do it later.

Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

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

@TG1999 Ready to merge when you are ready
@gmathiou4 Thank you ++ ❤️

@TG1999
Copy link
Contributor

TG1999 commented Aug 3, 2023

@TG1999 the tests are failing... may be a skeleton issue? I see also #119 ... so I guess I'll approve and we can do it later.

@pombredanne CI is failing due to this sphinx-doc/sphinx#11381

@TG1999
Copy link
Contributor

TG1999 commented Aug 3, 2023

Thanks @gmathiou4 merging ❤️

@TG1999 TG1999 merged commit e5043a8 into aboutcode-org:main Aug 3, 2023
7 of 10 checks passed
@gmathiou4
Copy link
Contributor Author

@TG1999, Thanks for merging!
Could you please help me clarify, if "main"= "tag 0.9.7".
If not, does that means that always the latest version is the main branch?

@gmathiou4
Copy link
Contributor Author

@TG1999 & @pombredanne Could you please release a new tag with the latest changes?

@TG1999
Copy link
Contributor

TG1999 commented Aug 7, 2023

@gmathiou4 released v0.9.8 here https://pypi.org/project/python-inspector/0.9.8/

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.

3 participants