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

Refmt printer error when running on a large codebase #2118

Closed
thangngoc89 opened this issue Aug 7, 2018 · 10 comments

Comments

@thangngoc89
Copy link
Contributor

commented Aug 7, 2018

I ran refmt over my repo and here are some bugs I found (which totally prevent me from using refmt@3.3.2)

          <span className="EditorNav__button--saveIndicator ">
            (
                (switch (saveStatus) {
                | Pristine => ""
                | Saved => "Saved"
                | Saving => "Saving"
                | Unsaved => "Unsaved"
                })
              ->str
            )
          </span>

Get formatted into this

<span className="EditorNav__button--saveIndicator ">
            switch (saveStatus) {
            | Pristine => ""
            | Saved => "Saved"
            | Saving => "Saving"
            | Unsaved => "Unsaved"
            }
            ->str
          </span>
blocks
  ->(blocks => {"blocks": blocks})

to

blocks
  ->blocks => {"blocks": blocks}
<title> ((state.title == "" ? "untitled" : state.title)->str) </title>

to

<title> state.title == "" ? "untitled" : state.title->str </title>

Refmt doesn't understand this (the self.send part)

ReasonReact.Router.watchUrl(url =>
        Route.urlToRoute(url)->ChangeView->self.send
      );

Changed it into this as a work around

ReasonReact.Router.watchUrl(url =>
        Route.urlToRoute(url)->ChangeView->send
      );
@IwanKaramazow

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2018

Thanks for the bug report, there's already a PR open with fixes, we're working on it.

@cknitt

This comment has been minimized.

Copy link

commented Aug 8, 2018

I ran into similar issues, e.g.

let twitterUrl = Social.profiles |. Map.String.getExn("twitter") |. (p => p.Social.url);

becomes

let twitterUrl = Social.profiles->(Map.String.getExn("twitter"))->p => p.Social.url;

Also, there are many cases where superfluous () are added, e.g.

locale->(Js.String.substring(~from=0, ~to_=2))

or

db
->(version(1))
->(stores(Js.Dict.fromList([("installation", ""), ("settings", ""), ("incidents", "incidentId")])))

or

        LicenseList.licenses
        ->(
            mapListToReactElements(item =>
              <div key=item.name>
                /* ... */
              </div>
            )
          )
@anmonteiro

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

@cknitt My open PR (#2111) fixes your issues.

Furthermore, the parens are not superfluous. -> has different precedence from |..

So these are different:

/* these are the same */ 
locale->(Js.String.substring(~from=0, ~to_=2));
Js.String.substring(locale, ~from=0, ~to_=2);


/* these are the same, but different from the examples above */ 
locale->Js.String.substring(~from=0, ~to_=2)
(locale->Js.String.substring)(~from=0, ~to_=2)
@jaredly

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

Wait, so
locale |. Js.String.substring(~from=0, ~to_=2) (which is equivalent to Js.String.substring(locale, ~from=0, ~to_=2)
gets refmt'd to
locale->(Js.String.substring(~from=0, ~to_=2)) (which is equivalent to Js.String.substring(~from=0, ~to_=2, locale))?
That sounds like a bug.
I would expect local |. Js.String.substring(~from=0, ~to_=2) to be refmt'd to locale->Js.String.substring(~from=0, ~to_=2).

@anmonteiro

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

@jaredly sorry my example was misleading. it gets put as the first arg.

Edited for consistency.

@cknitt

This comment has been minimized.

Copy link

commented Aug 8, 2018

My expectation would have been the same as @jaredly's.

I.e. I would have expected something like

a |. b(c) |. d;

to reformat to

a->b(c)->d;

but it reformats to

a->(b(c))->d;

which feels weird.

@anmonteiro

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

I'm starting to think the parens are actually not needed, given that a->b(c) is effectively equivalent to a->(b(c)) because of currying.

But they're needed in this case: a->(b->c)

@IwanKaramazow

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

The Reason fast pipe -> is actually different than the |. infix operator from Bucklescript.
There's a range of things -> can do, that |. can't. For example precedence like foo->bar##baz.
You can think of -> as an "access"-thing like the . infoo.bar or ## in foo##baz.
If you write a.b.c[0], everyone reads (((a).b).c)[0].
-> behaves exactly the same, a->b->c[0] is ((a->b)->c)[0].
a->b(c) is parsed as (a->b)(c), after the bs ppx transform it'll be (b(a))(c). Since Ocaml is a curried language this is actually the same as b(a, c).
Because of the differences between |. as infix operator and -> being "access"-like (think .), we can't just drop the parens when converting |. in ->. Reason operates on the ast level, we can't make any assumptions other than the ast we just parsed.
However, if you're compiling Reason with Bucklescript, you can drop the parens yourself safely. We don't have the same luxury on native.

@cknitt

This comment has been minimized.

Copy link

commented Aug 9, 2018

@IwanKaramazow Thanks for the explanation. I wasn't aware I could remove the superfluous () manually (and refmt would keep it that way). That is of course fine with me! Maybe this information could be added to the release notes / instructions for the migration script.

@IwanKaramazow

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2018

All bugs have been fixed in master. Ping me if there are any new issues surfacing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.