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

Restore comma-separated columns to by= and key= #6049

Merged
merged 11 commits into from
Jul 11, 2024

Conversation

MichaelChirico
Copy link
Member

@MichaelChirico MichaelChirico commented Apr 4, 2024

As discussed in #4357, there's not an urgent need to break back-compatibility here. We might reconsider for data.table 2.0.

This PR restores the functionality removed by #6047, and adds a helper function to at least have a unified approach across the 3 call sites.

We don't need to revert / undo any of the suggested changes to downstream packages -- it's still a good practice to use the character vector approach in package code, and we mainly maintain the "CSV" approach as a convenience for interactive usage.

Copy link

codecov bot commented Apr 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.53%. Comparing base (a11a2e2) to head (cb9ad20).
Report is 8 commits behind head on master.

❗ Current head cb9ad20 differs from pull request most recent head a55ff7c. Consider uploading reports for the commit a55ff7c to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6049   +/-   ##
=======================================
  Coverage   97.52%   97.53%           
=======================================
  Files          80       80           
  Lines       14978    14916   -62     
=======================================
- Hits        14607    14548   -59     
+ Misses        371      368    -3     

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

@tdhock

This comment was marked as outdated.

@MichaelChirico

This comment was marked as outdated.

Copy link

github-actions bot commented Jul 10, 2024

Comparison Plot

Generated via commit 636e91b

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

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

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

@MichaelChirico MichaelChirico changed the title Revert "Deprecate key="a,b" and by="a,b"" Restore comma-separated columns to by= and key= Jul 10, 2024
@MichaelChirico
Copy link
Member Author

OK, updated this PR to be a reversion to bring back 1.15.0 behavior. Thanks @HughParsonage for pushing back on the breaking change :)

@jangorecki jangorecki removed their request for review July 10, 2024 20:54
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.

this is fine with me, thanks

@MichaelChirico MichaelChirico merged commit 2577576 into master Jul 11, 2024
3 of 4 checks passed
@MichaelChirico MichaelChirico deleted the revert-6047-deprecate-key-comma branch July 11, 2024 14:04
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