Extend 'StringParser' for passing initial positions#129
Extend 'StringParser' for passing initial positions#129gfngfn wants to merge 3 commits intonyuichi:masterfrom
Conversation
| val run : 'a Char.t Parser.t -> string -> 'a ((Char.t parse-error) list) result | ||
| % [run-with-init-position p init-pos s] runs parser [p] for input [s]. | ||
| % Here, [init-pos] stands for the initial position of the input. | ||
| val run-with-init-position : 'a Char.t Parser.t -> token-position -> string -> 'a ((Char.t parse-error) list) result |
There was a problem hiding this comment.
What about adding an optional argument to StringParser.run instead of introducing a new function?
At present we have no consensus whether we should treat adding an optional argument as a breaking change, but IMO such a change is not very critical and can be included in a minor version up.
There was a problem hiding this comment.
Yes, I have once tried to add an optional argument to StringParser.run at the first commit as you pointed out:
It is possible, however, that adding optional arguments cause a kind of critical breaking change, roughly because having optional arguments restricts partial applications using |> in some cases. Indeed, the commit above breaks regexp.satyg.
Although this is not the problem of optional arguments but that of the type given to |>, it seems safer not to add optional arguments for now IMHO.
There was a problem hiding this comment.
Hmm, I understood the point.
But I'm still hesitant to merge this change in its current form for the following reasons:
- I don't feel it is a rigid method to add
run-with-fooalong withrun. If we want to add another parameterbar, we would need to addrun-with-barandrun-with-foo-and-bar. - Providing this new function
run-with-init-positionis not essential. Since the StringParser module doesn't contain opaque types, users can (in theory) write their own version ofrun-with-init-positionusing functions provided by the Parser module. (Of course, I don't meanrun-with-init-positionis worthless, but I mean we will need to clarify the motivation for adding this function at the risk of (1)).
I should emphasize I have no strong objection (nor a pushing proposal of my own). But I'd like to hear comments from anyone and evaluate to what extent my concern matters in reality.
This PR adds
StringParser.run-with-initial-position, a variant ofStringParser.runfor passing the initial position of inputs.val run : 'a Char.t Parser.t -> string -> 'a ((Char.t parse-error) list) result + val run-with-initial-position : 'a Char.t Parser.t -> token-position -> string -> 'a ((Char.t parse-error) list) resultThis will be useful for reporting exact positions where an error occurred during parsing, especially during parsing at the preprocessing stage.