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

Update tests + various lints #922

Merged
merged 12 commits into from
Jan 31, 2024
Merged

Update tests + various lints #922

merged 12 commits into from
Jan 31, 2024

Conversation

olivroy
Copy link
Collaborator

@olivroy olivroy commented Jan 29, 2024

  • Update tests to use more expect_equal(object, expected), (part of 396 R6 impl #471)
  • More use of snake case in tests to help finalize transition. It seems to have worked without issues for many months now, congrats on the seamless transition to snake_case.
  • Use of "" instead of ''
  • Other lints flagged to me by lintr.

Copy link
Owner

@JanMarvin JanMarvin left a comment

Choose a reason for hiding this comment

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

Thank you! There were some changes that really made me smile.

I’m not sure about lengths() on lists. This might have been from R 4.0 and even though that’s now a long time ago, my employers IT department is very slow with non crucial software updates (we use SAS) and I had to run openxlsx2 on R 3.6 iirc just last year. Therefore please have a look.

I’d also like to see one or two tests that still use camel case while we still support it. Otherwise something might break unknowingly to us.

And, if I ever again see a PR with your name and the number of impacted files is larger than 5 and it’s not a roxygen update I seriously will close it without any hesitation 😜

R/utils.R Outdated Show resolved Hide resolved
R/wb_functions.R Outdated Show resolved Hide resolved
tests/testthat/test-class-comment.R Show resolved Hide resolved
This was referenced Jan 30, 2024
@olivroy
Copy link
Collaborator Author

olivroy commented Jan 30, 2024

Sorry, sometimes, I don't know where I am going and it is easy to identify things when scanning code and you know you're never gonna come back to it if you don't touch it immediately.

Let's hold off test changes for snake_case.

@JanMarvin
Copy link
Owner

It’s alright, I get that, but I still don’t like these hundreds of lines all over the place PR. But I trust your judgment, if you say these lines are justified, that’s almost enough reason for me to merge. I just wanted the lengths thing checked and one or two tests for the camel case arguments.

@olivroy
Copy link
Collaborator Author

olivroy commented Jan 30, 2024

It’s alright, I get that, but I still don’t like these hundreds of lines all over the place PR. But I trust your judgment, if you say these lines are justified, that’s almost enough reason for me to merge. I just wanted the lengths thing checked and one or two tests for the camel case arguments.

I'd rather have the smaller PRs merged first, then I will merge the conflicts and see if I still think this is worth it.

Copy link
Owner

@JanMarvin JanMarvin left a comment

Choose a reason for hiding this comment

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

Very well. Now that the lengths() problem is solved, I have no real objections. I'll still write expect_equal(exp, got), but feel free to merge. Also you haven't swapped it everywhere :)

Also, I'm going to do you a favor: I plan to add a na.strings - > na_strings conversion before the next release. Let me know if you want to tackle this, otherwise I'll have a look.

Thank you so much for your work! I really appreciate it, even though it must feel like trying to clean up a petulant child's room. Hopefully it's for a greater whole!

@olivroy
Copy link
Collaborator Author

olivroy commented Jan 31, 2024

Very well. Now that the lengths() problem is solved, I have no real objections.

lengths() seems to have appeared in R 3.2 however, but better safe than sorry.

I'll still write expect_equal(exp, got), but feel free to merge. Also you haven't swapped it everywhere :)

No problems!

Also, I'm going to do you a favor: I plan to add a na.strings - > na_strings conversion before the next release. Let me know if you want to tackle this, otherwise I'll have a look.

Are you sure if this is worth the effort? Maybe num_fmt vs numfmt is the one I forget the most at this point.

Thank you so much for your work! I really appreciate it, even though it must feel like trying to clean up a petulant child's room. Hopefully it's for a greater whole!

Well, on my own, I always write terrible code, and I hate cleaning after myself. i hope that cleaning after others will help me write cleaner code at first? maybe that is delusion.

I will merge with your blessing then!

@olivroy olivroy merged commit 938c686 into main Jan 31, 2024
9 checks passed
@olivroy olivroy deleted the sparklines branch January 31, 2024 13:16
@JanMarvin
Copy link
Owner

Regarding the length stuff: Well they say time flies when you’re having fun 😉 , but actually I was thinking about a behavior change in length which is why I coded it the way it is. Something like this in 4.2.3.

As "POSIXlt" objects may be “partially filled” and their list components meant to be recycled, length() now is the length of the longest component.

@JanMarvin
Copy link
Owner

Also, I'm going to do you a favor: I plan to add a na.strings - > na_strings conversion before the next release. Let me know if you want to tackle this, otherwise I'll have a look.

Are you sure if this is worth the effort? Maybe num_fmt vs numfmt is the one I forget the most at this point.

I intend to add something like gsub(".", "_", arguments) to standardization() and switch the default to na_strings. Since we've switched to snake case everywhere it began to feel strange.

Have typed numfmt so often, that I do not stumble on this one anymore. But there are some cases where we have these inconsistencies. Still I'm quite happy with the stability of the API. Would have thought that it would be noisier.

@olivroy
Copy link
Collaborator Author

olivroy commented Jan 31, 2024

Yeah, me too, that's why I'd rather have numfmt everywhere, but not too important I guess!

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.

2 participants