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

Unnecessary brackets inside substitute expressions #889

Open
kpagacz opened this issue Dec 29, 2021 · 4 comments
Open

Unnecessary brackets inside substitute expressions #889

kpagacz opened this issue Dec 29, 2021 · 4 comments

Comments

@kpagacz
Copy link

kpagacz commented Dec 29, 2021

This PR #876 supposedly fixes the issue of styler putting additional, unnecessary brackets inside substitute expressions. But having installed the latest version of styler from main, I am still experiencing the issue described in #873

Here is my reprex:

library(magrittr)
expr <- substitute(
  expr = {
    iris %>%
      exprA %>%
      exprB
  },
  env = list(exprA = call("summary"), exprB = call("print"))
)

Styler styles it to:

library(magrittr)
expr <- substitute(
  expr = {
    iris %>%
      exprA() %>%
      exprB()
  },
  env = list(exprA = call("summary"), exprB = call("print"))
)

Here's my sessionInfo:
image

@lorenzwalthert
Copy link
Collaborator

Thanks @kpagacz. Indeed it was fixed in #876, but only for code where the expression of concern was a direct child of substitute(), also see the unit test added there. You are right, we should fix it in general, which will probably require to set an attribute of the nest of substitute() and propagate it downwards. Fortunately, this seems to work for rules that change tokens, beacuse they use pre_visit(), i.e. go from outside inwards (i.e. style substitute(...), then its content. Not sure this also works for post_visit(). I can't fix it immediately, so I advise you to be just turn {styler} off for that section. This is also the safest option since even once we fix the problem and release a new version, other people using your code might still use an old version of {styler} and break the code.

library(magrittr)
expr <- substitute(
  expr = {
  # styler: off
    iris %>%
      exprA %>%
      exprB
  },
  # styler: on
  env = list(exprA = call("summary"), exprB = call("print"))
)

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented Dec 29, 2021

Actually this does not work either because of another bug 😮‍💨... I filed closed #890. Should work with styler dev now.

@lorenzwalthert
Copy link
Collaborator

@kpagacz with #891, you should now at least be able to turn styler off for that sequence.

@kpagacz
Copy link
Author

kpagacz commented Dec 30, 2021

Having had a look at the unit tests and the styler code, I think I now understand why #876 does not work as I suspected.

Thanks a lot for the feedback. I actually filed this issue because styler: off and styler: on threw errors for me when I tried using it with substitute, so I felt like I was trapped to suffer manually reverting styler changes for every substitute occurrence.

I have tested on the latest main branch and I can now use # styler: off to turn styler off for particular lines as well as whole expressions in substitute. This is good enough for me and my team I think.

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

No branches or pull requests

2 participants