Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gfngfn
Copy link
Contributor

@gfngfn gfngfn commented Dec 20, 2020

This PR adds StringParser.run-with-initial-position, a variant of StringParser.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.

@gfngfn gfngfn changed the title [WIP] Extend 'StringParser.run' for passing initial positions [WIP] Extend 'StringParser' for passing initial positions Dec 20, 2020
@gfngfn gfngfn marked this pull request as ready for review December 20, 2020 09:30
@gfngfn gfngfn changed the title [WIP] Extend 'StringParser' for passing initial positions Extend 'StringParser' for passing initial positions Dec 20, 2020
@@ -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
Copy link
Owner

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.

Copy link
Contributor Author

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.

Copy link
Owner

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:

  1. I don't feel it is a rigid method to add run-with-foo along with run. If we want to add another parameter bar, we would need to add run-with-bar and run-with-foo-and-bar.
  2. 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 of run-with-init-position using functions provided by the Parser module. (Of course, I don't mean run-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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants