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

Fix printing & parsing of nested uncurried syntax. #1832

Merged
merged 2 commits into from
Feb 22, 2018

Conversation

IwanKaramazow
Copy link
Contributor

Fixes #1823

Parses/prints add(. 1, .2) correct as (add(. 1))(. 2)

Parses/prints `add(. 1, .2)` correct as `(add(. 1))(. 2)`
@IwanKaramazow
Copy link
Contributor Author

IwanKaramazow commented Feb 21, 2018

Still one case failing, need to fix the locations in parsing.

@chenglou chenglou requested a review from cristianoc February 21, 2018 22:09
Copy link
Contributor

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great thanks.
Taking the location range from the arguments should take care of the failing test.

let expr = if args = [] then body
else
let (args, wrap) = process_underscore_application args in
let expr = mkexp (Pexp_apply (body, args)) in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about taking the location range from the arguments:

            let args_loc = match args, List.rev args with
              | ((_, s)::_), ((_, e)::_) -> mklocation s.pexp_loc.loc_start e.pexp_loc.loc_end
              | _ -> assert false in
            let expr = mkexp (Pexp_apply (body, args)) ~loc:args_loc in

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the code, exactly what was needed.

The start & end location of the args form natural candidates for
the location of the corresponding Pexp_apply.
@chenglou
Copy link
Member

M-M-MERGED

@chenglou chenglou merged commit 9fcfac5 into reasonml:master Feb 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants