refactor: improve pathlib usage and add link parser tests#500
refactor: improve pathlib usage and add link parser tests#500biryazilimciysf wants to merge 4 commits into
Conversation
|
Hello! The PR is ready for review. All local tests and CI checks are passing. Please let me know if you need any changes. Thanks! |
| - id: validate-examples-changed | ||
| name: Validate JSON examples (changed docs) | ||
| entry: python3 scripts/validate_examples.py --schema-base source/schemas/ --file | ||
| entry: python scripts/validate_examples.py --schema-base source/schemas/ --file |
There was a problem hiding this comment.
just confirming, the python3 -> python change in pre-commit is for Windows compatibility?
There was a problem hiding this comment.
Hi! Yes, that's exactly correct. Changing python3 to python ensures better cross-platform compatibility, especially for Windows users where the alias is typically just python.
Thank you for catching this and for your contribution to the UCP project! Merging this now.
|
Hi @biryazilimciysf, thank you for this PR! The Windows encoding fixes and the new unit tests for the link checker are great improvements. I noticed that one of the new tests, test_multiple_bare_urls_with_quotes, is currently failing: AssertionError: 'https://ucp.dev/b' not found in ['https://ucp.dev/a'] Additionally, you might need to rebase your branch on the latest main to pull in recent changes. Could you please fix and update the PR? Thanks again for your contribution! |
d82a475 to
5db2243
Compare
|
Hi @damaz91 , thanks for the review and the helpful feedback! I've addressed all the points you raised: The failing test (test_multiple_bare_urls_with_quotes) is now fixed. |
| run: | | ||
| python -m pip install --upgrade pip | ||
| pip install requests mkdocs | ||
| # Projenin kendi gereksinimleri varsa kuruyoruz |
There was a problem hiding this comment.
can you please ensure all comments in this file are in english? thanks!
There was a problem hiding this comment.
:) Sure, my bad, good eye.
There was a problem hiding this comment.
this file looks redundant to me; aren't the tests already triggered in .pre-commit-config.yaml?
There was a problem hiding this comment.
That's a great question, thanks for asking! It brings up an important point about our testing strategy.
While it might seem redundant at first, the two validation steps serve distinct purposes:
validate_examples.py is our integration test. It runs against the entire documentation corpus to ensure all real examples are valid.
test_validate_examples.py is the unit test for the validator itself. It uses small, crafted inputs to verify that the validator's logic correctly enforces all the rules and handles edge cases (e.g., rejecting non-conformant examples).
The docstring in test_validate_examples.py puts it well: "The doc corpus is the integration test... This file is the unit test layer."
We run both in CI to be thorough. The unit tests ensure the tool is working, and the integration test ensures the documentation is correct using that tool. While some of these checks might also be in pre-commit for local developer convenience, the CI pipeline is our ultimate guarantee of quality.
Hope this clarifies things!
There was a problem hiding this comment.
still, can you use docs.yml to accomplish this? like https://github.com/Universal-Commerce-Protocol/ucp/blob/main/.github/workflows/docs.yml#L73C1-L74C1
There was a problem hiding this comment.
Thanks for the feedback! I've implemented the requested changes and pushed the updates to the fix-windows-encoding branch. The PR is ready for another review.
Description
What does this PR do?
encoding="utf-8"inhooks.pyfor file writing operations to prevent platform-specific encoding issues.pathlibusage inhooks.pyandscripts/check_links.pyto be more idiomatic.scripts/check_links.pyby avoiding overly broadexcept Exception:blocks and replacing TOCTOU patterns with EAFP.scripts/test_check_links.py) to verify the behavior of theLinkParser.Category (Required)
Please select one or more categories that apply to this change.
ucp-schematool (resolver, linter, validator). (Requires Maintainer approval)Related Issues
N/A
Checklist
!for breaking changes).Screenshots / Logs (if applicable)
N/A