WIP: Add requirement parsing module. #7020
Conversation
chrahunt
added
skip news
This prevents a circular import since req.constructors will depend on req.parsing.
To start, we use the parsed link instead of populating it in several places. We no longer need to normalize relative URLs since the requirement string must be either: 1. a relative path (e.g. `.` or `./example`), which gets normalized by `parse_requirement_text`, or 2. an absolute file URL
Since the lower-level parsing already determined it's a file path, we can just check whether it's a directory.
A single '=' may only be applicable to direct references or named references, since they are the ones that allow version specifiers.
Previously, non-existent file paths were falling through to the Requirement parsing and it was failing at that point. Now we specifically require that any mentioned files exist.
chrahunt
force-pushed
the
issue-7019
branch
from
577f842
to
87511e5
Compare
chrahunt
force-pushed
the
issue-7019
branch
from
87511e5
to
a1548fa
Compare
|
Can you separate behavior changes from refactoring? It’s very hard to evaluate when both are happening at the same time (changes in logic combined with moving stuff around, etc). |
|
Sure. For cases where an intermediate state would not be valid (e.g. some block gets moved above a declaration), would you prefer a comment saying something like "not valid, will be changed shortly" above the relocation or do you assume that something that looks wrong in one commit will be resolved in the next? |
|
By “invalid,” do you just mean style issues that might not normally pass review muster? In those cases, I think you can address it in comments in the PR (e.g. “don’t worry about method ordering in this PR because I’m planning to address that in a subsequent PR”). If it’s going to be addressed shortly, I don’t think we need to include it in the code itself IMO. |
|
When you said
I assumed you meant in separate commits. So one commit for a move and then one commit for the change, which makes sense to me. By "invalid" I meant if commit A moves some code but maybe it is broken in some obvious way (for example, moving a block of code above the initialization of a variable it needs), is that OK as-is if it gets fixed in B when the actual change occurs? |
|
I think the code should be working in each commit. For example, in a refactoring PR, each commit would have the same behavior as the previous (and in particular the code would still be working). I think refactoring changes should be in their own PR's separate from behavior changes (especially large changes). Separate from that, within a refactoring PR, it can help to put e.g. function moves and changes to the body of a function in separate commits. (That way you can see what is changing.) But when function moves, (behavior-preserving) refactoring changes, and behavior changes are all happening all at once, it makes everything that much harder to review and discuss. |
|
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
|
Still planning on going this direction, but more carefully. Thanks for the feedback @cjerdonek! |
lock
bot
added
the
auto-locked
This PR adds a new module:
req.parsingreq.parsingencapsulates the "pure" parsing logic needed to map incoming user requirements to something meaningful (i.e.RequirementInfo). This also makes it more clear what we accept in terms of syntax. This has the following changes in behavior:file://and be absolute - originallyfile:./relative/pathwas acceptedfile://paths must exist (previously paths that looked like archive files did not need to exist)For now only
install_req_from_linehas been updated. If this looks fine theninstall_req_from_editableandinstall_req_from_req_stringcan be done in a followup PR.Progresses #7019.