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

Use R 3.2.0 features #5838

Merged
merged 5 commits into from
May 4, 2024
Merged

Use R 3.2.0 features #5838

merged 5 commits into from
May 4, 2024

Conversation

MichaelChirico
Copy link
Member

@MichaelChirico MichaelChirico commented Dec 20, 2023

Newly-available functions taken from here:

https://github.com/r-lib/lintr/blob/f00f4a9a715346a3e696aab9cffff2756833e50a/R/backport_linter.R#L152-L159

Most significant are lengths(), trimws(), and strrep().

R/data.table.R Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.48%. Comparing base (6f3fc8d) to head (4469b48).
Report is 34 commits behind head on master.

❗ Current head 4469b48 differs from pull request most recent head 3cf6328. Consider uploading reports for the commit 3cf6328 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5838      +/-   ##
==========================================
- Coverage   97.51%   97.48%   -0.04%     
==========================================
  Files          80       80              
  Lines       14979    14859     -120     
==========================================
- Hits        14607    14485     -122     
- Misses        372      374       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jangorecki jangorecki added this to the 1.16.0 milestone Dec 20, 2023
@MichaelChirico MichaelChirico changed the base branch from master to 1-15-99 January 8, 2024 14:49
@MichaelChirico
Copy link
Member Author

@jangorecki anything to change here before merging to 1-15-99?

@jangorecki
Copy link
Member

In some PRs I recall using 3.1's (vapply's) lengths(). It may be better re-run lint/grep after merging PRs in the queue and catching all usage, not just current master.

@MichaelChirico
Copy link
Member Author

Still it would be good to turn on 3.2.0 ASAP to allow using the new functions. WDYT about this approach:

  1. This PR simply flips the DESCRIPTION dep
  2. Next PR turns on CI for linting
  3. Then we apply the rest of this PR, i.e. switching to 3.2.0+ "style" internally
  4. Turn on a linter for preventing backsliding

That way, we're "allowed" to use 3.2.0 sooner, but only "enforce" doing so all at once.

@MichaelChirico MichaelChirico mentioned this pull request Jan 12, 2024
@MichaelChirico MichaelChirico changed the title Move to R3.2.0 Use R 3.2.0 features Jan 12, 2024
Base automatically changed from 1-15-99 to master February 18, 2024 18:40
Copy link

github-actions bot commented Apr 23, 2024

Comparison Plot

Generated via commit 3cf6328

Download link for the artifact containing the test results: ↓ atime-results.zip

Time taken to finish the standard R installation steps: 11 minutes and 27 seconds

Time taken to run atime::atime_pkg on the tests: 3 minutes and 15 seconds

@MichaelChirico
Copy link
Member Author

@tdhock this is ready for review now that we have linting in CI. #6100 (downstream of this in Graphite stack) has the custom linter referenced to help prevent backsliding.

Copy link
Member Author

MichaelChirico commented May 3, 2024

@jangorecki jangorecki removed their request for review May 3, 2024 05:34
MichaelChirico and others added 2 commits May 3, 2024 11:50
* Fix 5492 by limiting the costly deparse to `nlines=1`

* Implementing PR feedbacks

* Added  inside

* Fix typo in name

* Idiomatic use of  inside

* Separating the deparse line limit to a different PR

---------

Co-authored-by: Michael Chirico <chiricom@google.com>
sritchie73 and others added 3 commits May 3, 2024 11:50
* Updated documentation for rbindlist(fill=TRUE)

* Print NULL entries of list as NULL

* Added news item

* edit NEWS, use '[NULL]' not 'NULL'

* fix test

* split NEWS item

* add example

---------

Co-authored-by: Michael Chirico <chiricom@google.com>
Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>
Co-authored-by: Benjamin Schwendinger <benjamin.schwendinger@tuwien.ac.at>
@MichaelChirico
Copy link
Member Author

Thanks for the reviews @tdhock, this PR is also part of the same "stack" -- I can submit them all at once with your review here. 🎉

Copy link
Member

@tdhock tdhock left a comment

Choose a reason for hiding this comment

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

looks good thanks

Copy link
Member Author

MichaelChirico commented May 4, 2024

Merge activity

@MichaelChirico MichaelChirico merged commit 8fdd408 into master May 4, 2024
4 checks passed
@MichaelChirico MichaelChirico deleted the r32 branch May 4, 2024 16:14
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.

None yet

5 participants