pola-rs / r-polars

Bring polars to R
https://pola-rs.github.io/r-polars/
Other
405 stars 35 forks source link

Clean tests #120

Closed etiennebacher closed 1 year ago

etiennebacher commented 1 year ago

Close #111

Mostly styling, replacing some expectation functions by other which are more suited, and add some regex for errors whose message is unlikely to change. I gave up styling the chains (cf #113) because there are just too many and there's no automated way to do it (e.g styler).

I've been through half of the tests so far and the ~25% coverage reported in #111 seems largely underestimated to me (probably due to the strange construction of functions), there is an amazing amount of tests.

Still WIP (stopped at test-expr_datetime.R, included), do not pay attention to potential test failures for now

sorhawell commented 1 year ago

@etiennebacher bare with me, a real big personal hobby-horse coming up :)

I am personally a fan of data.table and love that the code base is written predominantly with the =assignment operator. I think some parts of the R community have been pushing the <- to an extend, where it is implicitly hinted that the = operator is an inherently defunct operator and a code smell.

I would be proud if some belittled R user one day would try to counter an argument by saying "it is really ok, see r-polars does it too".

How about tests are free game, but the translations predominantly uses = ? :)

I searched many time but ever only seen below two code examples to argue "=-bad", which I have not yet understood is an issue, because both behaviors are discouraged by the same style guides anyways.

> x=1
> z <- y = x
Error in z <- y = x : could not find function "<-<-"

and because of this

 > f = function(x) print(x)
> f(x=2)
[1] 2
> f(x<-4)
[1] 4
> x
[1] 4

what do you think? :)

etiennebacher commented 1 year ago

How about tests are free game, but the translations predominantly uses = ? :)

I don't have any problems with using = instead of <- in the code (I used to prefer <- because that's what I was taught but I saw many popular packages use = like data.table so I don't really care anymore ;) ).

The thing is simply that I ran styler::style_dir() on the tests to fix some styling (add a space after a comma, or before and after a =, etc.) and it also replaced = by <-. I will revert this change at the end, when this PR is ready for review.

eitsupi commented 1 year ago

FYI, there is a well-known package knitr that uses = as the assignment operator, and I believe the knitr code is formatted with formatR. https://github.com/yihui/formatR

sorhawell commented 1 year ago

I do like formatters. I have not used it in R yet. I will try it out :)

eitsupi commented 1 year ago

I think that the styler is probably more popular, although it can probably be configured to use = for the assignment operator, but I rarely write settings for styler because I have to write the settings in R scripts. https://stackoverflow.com/questions/50660401/use-instead-of-for-assignment-when-styling-r-code-with-styler

vincentarelbundock commented 1 year ago

I searched many time but ever only seen below two code examples to argue "=-bad", which I have not yet understood is an issue, because both behaviors are discouraged by the same style guides anyways.

@sorhawell Here's a video of Ross Ihaka saying we should all be using =. Thought you might get a kick out of that 😉

https://www.youtube.com/watch?v=88TftllIjaY&t=2096s

eitsupi commented 1 year ago

I prefer to let the auto-formatter (styler) do the work rather than obsessing over something. I don't think there is much value in getting hung up on manual formatting here.

etiennebacher commented 1 year ago

There's actually not much useful in this PR, most of it is styling and indeed it's better to let styler or something else do it. Probably better to close this, there are other priorities (but I let #111 open)

sorhawell commented 1 year ago

@vincentarelbundock

Thought you might get a kick out of that

Good video also :) I have dream of moving to NZ for two years.

<- people and = people, all this infighting is unnecessary. We we have so much in common being "left assignment oriented", together we can agree that the "right assignment" -> people are the odd ones! 😆

sorhawell commented 1 year ago

@eitsupi

I will try to use styler.equals + rstudio addin

https://github.com/Robinlovelace/styler.equals/issues/1

eitsupi commented 1 year ago

@eitsupi

I will try to use styler.equals + rstudio addin

Robinlovelace/styler.equals#1

Wonderful! I could not find it.