-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extend 'StringParser' for passing initial positions #129
base: master
Are you sure you want to change the base?
Conversation
@@ -287,6 +287,9 @@ end | |||
|
|||
module StringParser : sig | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-foo
along withrun
. If we want to add another parameterbar
, we would need to addrun-with-bar
andrun-with-foo-and-bar
. - Providing this new function
run-with-init-position
is not essential. Since the StringParser module doesn't contain opaque types, users can (in theory) write their own version ofrun-with-init-position
using functions provided by the Parser module. (Of course, I don't meanrun-with-init-position
is 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.run
for 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) result
This will be useful for reporting exact positions where an error occurred during parsing, especially during parsing at the preprocessing stage.