Enhance code quality and robustness: fixes for pattern context, parser loop, namespace validation, week validation, and bitflags#287
Conversation
gir-bot
added
S: needs-review
|
I probably would like to see these broken up, but additionally, tests defined that show the use cases you are fixing to help justify the changes. |
|
Just taking a quick look:
|
|
Also, changelog entries would be appreciated. |
|
It looks like the changes break a few week These changes need to be fully tested, and if an existing test needs to be changed, we need to be very sure the original case was wrong. From what I can tell, none of the failing tests should be failing. It seems the proposed changes actually introduce some regressions. So breaking these changes up and validating separately is definitely my preferred approach. I think some of these changes may not be desired. |
|
It seems our |
|
I'm fairly certain the failure |
|
The infinite loop, I suspect, doesn't actually occur in the wild. I'm thinking, if it does, it should likely throw an exception. |
facelessuser
left a comment
facelessuser
left a comment
There was a problem hiding this comment.
Since I started digging into this, I think we can address everything in this PR without splitting. I hadn't planned to do a deep dive on this, but I did.
| # Some patterns require additional logic, such as default. We try to make these the | ||
| # last pattern, and append the appropriate flag to that selector which communicates | ||
| # to the matcher what additional logic is required. | ||
| # Preserve any flags that were set during parsing (e.g. :empty, :root) |
There was a problem hiding this comment.
I would need to see evidence via new test(s), showing why this is needed.
| col = index - last + 1 | ||
| elif last <= index < m.end(0): | ||
| indent = '--> ' | ||
| offset = (-1 if index > m.start(0) else 0) + 3 |
There was a problem hiding this comment.
I'm not convinced this fixes anything, but tests show it regresses one of our tests. I'm open to making a change if a real case can be demonstrated, but it seems like the logic would have to be altered to keep the existing tests working and solve the new test case, if one was shown to need fixing.
|
I've actually picked out the verified fixes from this PR in #288. I've added appropriate tests and such and will merge them separately. That leaves us just with two things:
|
|
Thanks @facelessuser for your valuable feedback on this PR. I'm very happy to see that you already merged some of the suggested changes. I'm very grateful for your time spent on this project. I'll need a few days for a deep dive to answer your questions. Thanks again for your support! |
|
@mundanevision20 I'm going to close this PR as it is now out of date. If your investigations into the other two issues end up turning up real-world problems, feel free to open up a new PR or issue so we can evaluate a path forward. I will probably go ahead and publish a release just to ensure the fix for the infinite loop problem gets out there, as it is the more serious issue. |
3 participants
Hi @facelessuser ,
thank you for maintaining soupsieve; I appreciate your work.
Suggested changes
The targeted fixes informed by reproduced edge cases and investigation notes to address incorrect offsets, potential parser non-termination on malformed input, namespace validation gaps, week validation edge cases, and an incorrect bitflag assignment. Changes are minimal and focused on root causes.
I'm happy to split this PR into smaller PRs or add a changelog entry if preferred.