Conversation
Updated version extraction logic from requirements.txt to use regex.
There was a problem hiding this comment.
Pull request overview
Refactors the CLI’s requirements.txt version detection to support PreTeXt requirements with extras (e.g., pretext[prefigure] == 2.36.0), addressing issue #1115 where such lines were previously mishandled.
Changes:
- Adds regex-based parsing for
pretext/pretextbookpinned versions inrequirements.txt. - Updates logic for extracting the pinned version from matching requirement lines.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
pretext/utils.py
Outdated
| with open(pp / "requirements.txt", "r") as f: | ||
| REGEX = r"\s*pretext(book)?(\[.*\])?\s*==\s*(?P<version>[\d\.]*)\s*"gm | ||
| for line in f.readlines(): | ||
| if ("pretext ==" in line) or ("pretextbook ==" in line): | ||
| return line.split("==")[1].strip() | ||
| if re.match(REGEX, line): | ||
| return re.match(REGEX, line).group("version") |
There was a problem hiding this comment.
requirements_version() parsing logic changed, but there are no targeted tests covering supported requirement lines (e.g., pretext[prefigure] == 2.36.0, pretextbook == ..., whitespace variations). Add a unit test to lock in the intended matching behavior and prevent regressions.
pretext/utils.py
Outdated
| import logging | ||
| import logging.handlers | ||
| import psutil | ||
| import re |
There was a problem hiding this comment.
re is imported twice in this module (already imported earlier). This will trip linters and is unnecessary; remove the newly added duplicate import.
| import re |
pretext/utils.py
Outdated
| return None | ||
| try: | ||
| with open(pp / "requirements.txt", "r") as f: | ||
| REGEX = r"\s*pretext(book)?(\[.*\])?\s*==\s*(?P<version>[\d\.]*)\s*"gm |
There was a problem hiding this comment.
REGEX = r"..."gm is invalid Python syntax (it will raise a SyntaxError at import time). If you intended regex flags, pass them via re.compile(..., flags=...) (e.g., re.MULTILINE) instead of appending gm to the literal.
| REGEX = r"\s*pretext(book)?(\[.*\])?\s*==\s*(?P<version>[\d\.]*)\s*"gm | |
| REGEX = r"\s*pretext(book)?(\[.*\])?\s*==\s*(?P<version>[\d\.]*)\s*" |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@oscarlevin I've opened a new pull request, #1122, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Initial plan * Fix review comments: remove duplicate import, fix regex syntax, add tests Co-authored-by: oscarlevin <6504596+oscarlevin@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: oscarlevin <6504596+oscarlevin@users.noreply.github.com> Co-authored-by: Oscar Levin <oscar.levin@gmail.com>
Closes #1115
Hacked this out on GitHub.com so who knows if it works lol