Replace ParamList with Group #2142
Conversation
| # res.update(t) | ||
| if isinstance(t, ParseResults): | ||
| for i in t: | ||
| if isinstance(i, ParamValue): |
There was a problem hiding this comment.
Could it make sense to raise an exception if this condition does not match?
There was a problem hiding this comment.
Yeah I think that's a lot clearer, as otherwise it looks like we're filtering our for ParamValues, but they should all be ParamValues. Fixed in latest commit.
There was a problem hiding this comment.
Raising a TypeError here conflicts with this line in the docstring: "Any sub-tokens that are not Params will be ignored." So raising TypeError here may start breaking some existing code.
There was a problem hiding this comment.
I completely missed that! I'm not sure how true that line in the docstring is, but I think it's worth an investigation in a separate PR.
There was a problem hiding this comment.
@veyndan thanks for the PR and apologies for the late review. It looks correct to me but I'm going to try get someone with some knowledge of pyparsing to also weigh in and do another review after thinking about it a bit, but I think this approach is significantly better than using a custom class.
veyndan
force-pushed
the
rm-ParamList
branch
from
03c2ae7 to
c79ecc2
Compare
|
@aucampia Is this okay to merge? |
|
I have a few more comments to make on the use of Group, which I'll try to write up this evening. |
|
It has been a hairy few weeks, I'm sorry this took so long. I've started a new page in the pyparsing wiki for pyparsing design topics, with the first topic about using |
Summary of changes
No functional changes. I've wanted to better understand the parsing of SPARQL, and have found that
ParamListmakes the parsing unnecessarily more complex. ReplacingParamListwith the built-inGroupoperator simplifies the ability to understand how a SPARQL query is being parsed.In order to replace
ParamListwithGroup, I just did a find and replace ofParamList(foo)withGroup(Param(foo)). Perhaps replacing instances ofZeroOrMore(ParamList(foo))should be replaced withGroup(ZeroOrMore(Param(foo))instead ofZeroOrMore(Group(Param(foo)), but I'm not too sure of the benefits of one over the other, so I instead opted for what semantically matches what we had prior to this change.Checklist