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

Add equals_style() #451

Closed
wants to merge 1 commit into from
Closed

Conversation

Robinlovelace
Copy link

Rebased from master.

@Robinlovelace
Copy link
Author

Copied-over from #449

@lorenzwalthert
Copy link
Collaborator

To do after merge: reorganize file style-guides into smaller files.

#'
#' @details
#'
#' This style guide is the same as [`tidyverse_style()`], except it uses
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can drop the backticks. in tidyverse_style()

Copy link
Collaborator

Choose a reason for hiding this comment

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

The description is not true with the current implementation, as it does not inherit from the tidyverse style guide. Can you adapt that?

#'
#' Use equals assignment instead of arrow assignment.
#'
#' @inheritParams tidyverse_style
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to inherit from the tidyverse style guide? I don't see how they are related.

@lorenzwalthert
Copy link
Collaborator

Sorry @Robinlovelace for some reason these comments were just posted now because I clicked start a review and never finished instead of just commenting one-off. I wrote them a few weeks ago :-(

@Robinlovelace
Copy link
Author

Hi @lorenzwalthert thanks for the comments. I also just saw these! Will aim to look this evening but not guarantees...

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented May 12, 2019

Because of #513, I am not sure if we should ever replace <- with =.

@Robinlovelace
Copy link
Author

Yes in edge cases were <- is used in a function call replacing with = can fail. What are your thoughts on this? In some ways <- to = is easier than = to <- because a global search and replace will sort it (assuming the code has no edge cases where objects are assigned mid function call), so automation in a function is less important:

gsub(pattern = "<-", replacement = "=", x = "x <- mean(cars$speed, na.rm = TRUE)")

works.

Still I think it would be great to support styling to equals assignement.

@lorenzwalthert
Copy link
Collaborator

lorenzwalthert commented May 13, 2019

Well, it's rarely a good idea to work with regular expressions I think. It's rather slow, unreliable and does not fit the styler framework. We found a solution to #513 (just see the discussion there) and implmenented in this way: pat-s@5bf20ab. To use next_terminal(), make sure you merge r-lib/styler into your fork. Then it should work.

@pat-s pat-s mentioned this pull request May 13, 2019
@Robinlovelace
Copy link
Author

Great. Fully agree a non-regex solution would be ideal. Not sure how it relates to this PR though, I've lost track of where we were and next steps I'm afaid!

@lorenzwalthert
Copy link
Collaborator

Closed due to inactivity.

@Robinlovelace
Copy link
Author

Sensible decision, would you welcome a rejuvenation of this? Would be useful for ppl using the style in their work. I've been using styler to tidy up my code and replace = with <- in projects that use that, e.g.: ropensci/stplanr@33158a5

Would be great to be able to go in the opposite direction, e.g. here: https://github.com/ropensci/stats19/blob/master/inst/iow_example.R

@Robinlovelace Robinlovelace deleted the equals-style2 branch October 16, 2019 15:46
@lorenzwalthert
Copy link
Collaborator

Sorry, I did not mean you to delete the branch entirely. I should probably have asked about the status of this before closing. Sorry. Yes, you can create another PR.

@Robinlovelace
Copy link
Author

OK great. I think starting over (again!) may be the best way forward in general, but I'm sure of the specific next steps. Will see if there is scope to combine efforts with @pat-s on this. As always any pointers/thoughts welcome and no worries about closing the PR, it had definitely gone stale!

@pat-s
Copy link
Contributor

pat-s commented Oct 17, 2019

@Robinlovelace If you refer to the "mlr-style", @lorenzwalthert did most of the work. I essentially just tested and used it since then :) (and bother him once in a while with some upstream merge issues 😄 )

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

Successfully merging this pull request may close these issues.

3 participants