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.

Sign up for GitHub

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

Jump to bottom

Extend 'StringParser' for passing initial positions #129

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

Extend 'StringParser' for passing initial positions #129

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
78e65bd
add an optional argument to 'StringParser.run' for passing initial po…
gfngfn Dec 20, 2020
f9f9284
separate 'run-with-initial-position' from 'run'
gfngfn Dec 20, 2020
b85e4b8
update comments
gfngfn Dec 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions src/parser.satyg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

val char : Char.t -> Char.t Char.t Parser.t
val string : string -> string Char.t Parser.t % wrapped with `try`
val satisfy : (Char.t -> bool) -> Char.t Char.t Parser.t
Expand All @@ -310,7 +313,7 @@ end = struct
| (c :: cs) = char c >> aux cs
in try (aux (String.to-list str))

let lex-string input =
let lex-string init-pos input =
Stream.unfold (fun (cs, pos) -> (
match List.uncons cs with
| None -> Option.none
Expand All @@ -321,10 +324,13 @@ end = struct
let column = if Char.equal c Char.newline then 0 else pos#column + 1 in
(| line = line; column = column |)
in Option.some (token, (new-cs, new-pos))
)) (String.to-list input, Token.initial-position)
)) (String.to-list input, init-pos)

let run p input =
input |> lex-string |> Parser.run p
input |> lex-string Token.initial-position |> Parser.run p

let run-with-init-position p init-pos input =
input |> lex-string init-pos |> Parser.run p

let satisfy f =
Parser.satisfy (fun t -> f (Token.data t)) <&> Token.data
Expand All @@ -335,4 +341,4 @@ end = struct
let alnum = satisfy Char.is-alnum
let space = satisfy Char.is-space
let spaces = many space |> map String.of-list
end
end