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

remove unnecessary parens around float & integer constants in constructor expr/fun app #1634

Merged
merged 4 commits into from
Nov 18, 2017
Merged

remove unnecessary parens around float & integer constants in constructor expr/fun app #1634

merged 4 commits into from
Nov 18, 2017

Conversation

IwanKaramazow
Copy link
Contributor

fixes #1630

/* before */
Some((-1));

funAppl((-1));

/* after */
Some(-1);

funAppl(-1);

IwanKaramazow
IwanKaramazow commented Nov 11, 2017
View reviewed changes
formatTest/typeCheckedTests/expected_output/reasonComments.re
/* Retain this */
X(2, 3)
X
(2, 3) /* Retain this */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure why this happening

chenglou
chenglou reviewed Nov 11, 2017
View reviewed changes
src/reason-parser/reason_pprint_ast.ml Outdated
* pass through this case. In this context they don't need to be wrapped in extra parens
* Some((-1)) should be printed as Some(-1). This is in contrast with
* 1 + (-1) where we print the parens for readability. *)
self#constant ~parens:(false || ensureExpr) c
Copy link
Member

Choose a reason for hiding this comment

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

Can remove false

IwanKaramazow
IwanKaramazow commented Nov 11, 2017
View reviewed changes
formatTest/unit_tests/expected_output/syntax.re Outdated

Some(-0.1G, -0.1x, -0.1H);

Some([@foo] (-1), [@foo] (-1), [@foo] (-1));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's possible to not print the parens in this case, should we remove them?

Copy link
Member

Choose a reason for hiding this comment

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

I think so? The fewer the better =D

@jordwalke
Copy link
Member

Did you see there was a high pri bug someone found with unary negation and named arguments? Does this make those matters worse? Worth adding a test case (even if it has to be disabled)

@IwanKaramazow
Copy link
Contributor Author

Hmm I've seen it, this PR seems like a good opportunity to fix the high pri bug too.

@chenglou
Copy link
Member

Should I wait til you add more tests?

@IwanKaramazow
Copy link
Contributor Author

Yes, with a bit of a luck I can fix it too.

@IwanKaramazow
Copy link
Contributor Author

@chenglou this is ready, added extra tests & removed the extra parens when printing with attributes

chenglou
chenglou reviewed Nov 16, 2017
View reviewed changes
src/reason-parser/reason_pprint_ast.ml Outdated
let parens = if parens then (i.[0]='-') else false in
paren parens (fun f -> pp f "%s") f i
| Pconst_float (i, Some m) ->
let parens = if parens then (i.[0]='-') else false in
Copy link
Member

@chenglou chenglou Nov 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This can be simplified to parens && i.[0]='-' or maybe even inlined. Up to you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@chenglou
Copy link
Member

Merging! Thanks.

@chenglou chenglou merged commit edca12b into reasonml:master Nov 18, 2017
chenglou added a commit that referenced this pull request Nov 18, 2017
@chenglou
[Docs] #1634
14d5d84
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chenglou chenglou chenglou left review comments

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Printer: extra parentheses around variants with payload
4 participants
@IwanKaramazow @jordwalke @chenglou @facebook-github-bot