Validate website field#1219
Conversation
|
|
|
@1ucian0 @garrison @blakejohnson I tested the changes locally using pytest, pylint, and black formatting checks. The validation tests passed successfully, pylint reported a 10.00/10 score, and the file was reformatted using black to match repository style requirements. |
|
This is the latest update and final version of the PR. I improved the validation logic by normalizing hostnames before comparison, added handling for repository subpaths so URLs like |
|
No need to ping at people, the PR will get reviewed eventually :) |
1ucian0
moved this from PR open / Contributor working on it
to Waiting for maintainer response
in Contributor Monitoring
|
Thank you for letting me know. Sorry I didn't know about this. I am still learning how open source works. I won't ping reviewers in future PRs. Thanks for the guidance.. |
1ucian0
left a comment
1ucian0
left a comment
There was a problem hiding this comment.
Thanks! few comments here and there.
| continue | ||
|
|
||
| @staticmethod | ||
| def normalize_url(url): |
There was a problem hiding this comment.
Maybe you can bake these normalizatoins inside get_all_urls
There was a problem hiding this comment.
Thaks for the suggestion .Since get_all_urls() is also used by test_http(), would you prefer inlining the normalization into is_same_or_subpath(), or did you have a different approach in mind for get_all_urls()
| if package_url is None: | ||
| continue | ||
|
|
||
| hostname = (package_url.hostname or "").lower() |
There was a problem hiding this comment.
I think URL already lowers the case of hostname
| # Reject PyPI URLs as website | ||
| pypi_packages = getattr(member, "pypi", {}) | ||
|
|
||
| for _, package in pypi_packages.items(): |
There was a problem hiding this comment.
no need to fetch the python package. If the website is a pypi link, that's enough.
github-project-automation
Bot
moved this from Waiting for maintainer response
to Waiting for contributor
in Contributor Monitoring
Co-authored-by: Luciano Bello <766693+1ucian0@users.noreply.github.com>
| @staticmethod | ||
| def is_same_or_subpath(candidate, reference): |
There was a problem hiding this comment.
Maybe it can be implemented as an assertion:
| @staticmethod | |
| def is_same_or_subpath(candidate, reference): | |
| def assertSameSubpath(self, candidate, reference): |
Co-authored-by: Luciano Bello <766693+1ucian0@users.noreply.github.com>
3 participants
This PR adds a validation check to prevent redundant website URLs in submissions. The new validation rejects cases where member.website duplicates an existing GitHub repository URL or a PyPI project URL.
@1ucian0 @garrison @blakejohnson I also attempted to run the full local pytest suite, but test collection currently fails due to an unrelated existing syntax error in tests/test_check.py on the main branch. My changes are limited to ecosystem/validation/test_url.py.
Please let me know if this validation approach looks acceptable. If needed, I can also open a separate issue and work on fixing the pytest syntax error in tests/test_check.py as a follow-up contribution.
Fixes #1087