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

Set linter and auto formatter #214

Open
eitsupi opened this issue May 13, 2023 · 23 comments
Open

Set linter and auto formatter #214

eitsupi opened this issue May 13, 2023 · 23 comments
Labels
enhancement New feature or request
Milestone

Comments

@eitsupi
Copy link
Collaborator

eitsupi commented May 13, 2023

I tried styler.equals, but unfortunately, this currently seems to do little more than modify the assign operator.
So perhaps styler.equals::style_pkg() should be executed after styler::style_pkg().

@eitsupi eitsupi added the enhancement New feature or request label May 13, 2023
@eitsupi eitsupi changed the title Set linter and autoformatter Set linter and auto formatter May 13, 2023
@sorhawell
Copy link
Collaborator

I also ran regular styler first and then styler.equals in this Rstudio add-in.

https://github.com/sorhawell/styler.equals/blob/main/R/saveAndStylerEqualsAddin.R

@Robinlovelace
Copy link

Finally seeing this conversation. Based on what is in styler I'm unsure why styler.equals is failing to style packages. Glad you have found out issues with it, any ideas on solutions welcome...

@eitsupi
Copy link
Collaborator Author

eitsupi commented May 21, 2023

Based on what is in styler I'm unsure why styler.equals is failing to style packages.

Thanks for the reply. It seems you've pinpointed the cause in Robinlovelace/styler.equals#3 (comment).

@etiennebacher
Copy link
Collaborator

@eitsupi I think this can be closed?

@Robinlovelace
Copy link

Just got a notification on this. Seems it can be closed to me. On a different topic, I still didn't get styler.equals to work : ( any ideas how I can fix it? Thanks!

@eitsupi
Copy link
Collaborator Author

eitsupi commented Mar 21, 2024

@etiennebacher I don't think Rust and R linters are set up in CI yet.

@etiennebacher
Copy link
Collaborator

I didn't understand this was the point of this issue, I thought task format- and the new linter CI were enough

@lorenzwalthert
Copy link

lorenzwalthert commented Apr 5, 2024

Just got a notification on this. Seems it can be closed to me. On a different topic, I still didn't get styler.equals to work : ( any ideas how I can fix it? Thanks!

Maintainer of {styler} here, I can maybe help you if you open/point me to an issue in your repo and explain the problem to me. Actually, I think you could just take the code developed as part of this StackOverflow answer developed with my help in r-lib/styler#1170 if that's ok for the R Polars maintainers (and sorhawell in particular) from a licensing point of view.

@Robinlovelace
Copy link

Hey @lorenzwalthert I know you have already helped but would you be willing to provide some pointers on the shelved styler.equals package? I sense that could be an upstream fix, not 100% sure though. Thanks and apologies for jumping in on this thread.

@etiennebacher
Copy link
Collaborator

etiennebacher commented Apr 6, 2024

Thanks for the offer, but AFAIK the only reason this issue is open is because we need to setup a CI for styler. Currently we use styler and modify one of its transformers to prefer = over <- (see this script). Locally we just have to run task format-r

@Robinlovelace
Copy link

Heads-up @eitsupi I think this is fixed upstream in {styler.equals}. Can you check and confirm it work?

Will look to release to CRAN and put it out there if so!

@eitsupi
Copy link
Collaborator Author

eitsupi commented Apr 29, 2024

@Robinlovelace Thank you for working on that!
But as stated above, this repository is already not using styler.equals, so don't worry about me.

@lorenzwalthert
Copy link

lorenzwalthert commented Apr 29, 2024

I think {styer.equals}, as a packaged solution, is more lightweight to use than to define a style guide in this project (if you want tidyverse style + = over <-). Also, it integrates better with RStudio Addins for styling, pre-commit and other tools. In addition, it can correctly format edge cases the style guide developed here can't, e.g. c(a <- 1).

@eitsupi
Copy link
Collaborator Author

eitsupi commented Apr 29, 2024

Thanks for the suggestion. I will take a look when I have time.

@eitsupi
Copy link
Collaborator Author

eitsupi commented Apr 29, 2024

I was not concerned with the use of = in the first place, and did not want to deviate from the default setting of styler.
I was just respecting that because @sorhawell was preferring that.

But now that @sorhawell is not doing the development, I would prefer if I could simplify things by moving away from the use of =.

@etiennebacher Any thoughts?

@etiennebacher
Copy link
Collaborator

etiennebacher commented Apr 29, 2024

I don't have a preference between <- and = and in the end I don't think choosing one or the other matters. There are some cases where <- works and = doesn't, e.g if( a <- 1), but I don't like this kind of code as I find it less readable (and I never had to review code with this kind of pattern in this package). = is used in other (widely) used packages so this is not a problem.

We have something that works, I don't think we should spend too much time on this. That said, if you want to change, I have nothing against it.

@eitsupi
Copy link
Collaborator Author

eitsupi commented Apr 29, 2024

Since I usually work on VS Code using the languageserver package (based on styler) as a formatter, auto-formatting with commands in the editor will use <- due to the default setting of styler (it will also replace any existing =).

I then use the script to replace them with = later, which is not a big hassle, but it is definitely a hassle.

@lorenzwalthert
Copy link

Last comment from my side here: VS Code is a good example for integration with other tools. Now that a packaged style guide exists with {styler.equals} that does what you want (if you still want 😄), you can simply

options(languageserver.formatting_style = function(options) {
    styler.equals::equals_style(scope = "indention", indent_by = options$tabSize)
})

as per the docs of the language server.

@Robinlovelace
Copy link

Awesome!

@eitsupi
Copy link
Collaborator Author

eitsupi commented Apr 30, 2024

Last comment from my side here: VS Code is a good example for integration with other tools. Now that a packaged style guide exists with {styler.equals} that does what you want (if you still want 😄), you can simply

Thanks for pointing that, looks great!
I will try that.

@sorhawell
Copy link
Collaborator

sorhawell commented Jul 4, 2024

I got a new job and small kids and will most likely not be contributing to any open source for two years or so.

All power to the active developers ! :) = is dead, long live <-

My secret next open source goal is to introduce <-as assignment operator in a python project.

@Robinlovelace
Copy link

I think we can close this for now then (also speaking with 1 soon to be 2 lottle ones, good luck with it all and thanks for getting the ball rolling on this and supporting the project as it evolves)!

@eitsupi
Copy link
Collaborator Author

eitsupi commented Jul 5, 2024

Thanks for looking into this.
I will try to set up a CI with the work on the next branch (maybe pre-commit?), which is still relatively small.

@eitsupi eitsupi added this to the Rewrite milestone Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants