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.

Sign up for GitHub

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

Jump to bottom

merge 1-15-99 branch into master #5937

Merged
merged 10 commits into from
Feb 18, 2024
Merged

merge 1-15-99 branch into master #5937

merged 10 commits into from
Feb 18, 2024

Conversation

tdhock
Copy link
Member

@tdhock tdhock commented Feb 15, 2024

hi @MichaelChirico @jangorecki I believe now we can merge the dev branch 1-15-99 into master, right?

@codecov Codecov
Copy link

codecov bot commented Feb 15, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bf49909) 97.48% compared to head (8e2cc18) 97.48%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5937      +/-   ##
==========================================
- Coverage   97.48%   97.48%   -0.01%     
==========================================
  Files          80       80              
  Lines       14862    14857       -5     
==========================================
- Hits        14488    14483       -5     
  Misses        374      374

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

@MichaelChirico
Copy link
Member

Not quite, we need to rebase first, not merge. I've just --force pushed the result of rebase, PTAL and make sure the history looks correct.

Crucially, we should not squash commits during this merge so that each commit (which was squashed from a PR into the 1-15-99 branch) survives to master.

MichaelChirico
MichaelChirico reviewed Feb 17, 2024
View reviewed changes
NAMESPACE Outdated
@@ -51,6 +51,7 @@ S3method(cube, data.table)
S3method(rollup, data.table)
export(frollmean)
export(frollsum)
export(frollmax)
Copy link
Member

Choose a reason for hiding this comment

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

@jangorecki should we roll back #5889 before merging?

Copy link
Member

Choose a reason for hiding this comment

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

Probably better

Copy link
Member

Choose a reason for hiding this comment

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

OK, I hope it doesn't do anything too catastrophic with all the --force pushes going around. Since the diffs are still visible in the #5889 web UI, at worst we can reconstitute the changes manually.

Copy link
Member

Choose a reason for hiding this comment

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

NB: had to remove the #5910 changes to src/froll.c. Will need to reconstitute those.

@MichaelChirico MichaelChirico mentioned this pull request Feb 17, 2024
Merged
@MichaelChirico MichaelChirico mentioned this pull request Feb 17, 2024
Merged
OfekShilon and others added 8 commits February 17, 2024 01:08
@OfekShilon @MichaelChirico
Fix transform slowness (#5493)
0d24afb 
* 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>
@Anirban166 @MichaelChirico
Improvements to the introductory vignette (#5836)
dca5494 
* Added my improvements to the intro vignette

* Removed two lines I added extra as a mistake earlier

* Requested changes
@davidbudzynski @MichaelChirico
Vignette typo patch (#5402)
2a8fe7b 
* fix typos and grammatical mistakes

* fix typos and punctuation

* remove double spaces where it wasn't necessary

* fix typos and adhere to British English spelling

* fix typos

* fix typos

* add missing closing bracket

* fix typos

* review fixes

* Update vignettes/datatable-benchmarking.Rmd

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>

* Update vignettes/datatable-benchmarking.Rmd

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>

* Apply suggestions from code review benchmarking

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>

* remove unnecessary [ ] from datatable-keys-fast-subset.Rmd

* Update vignettes/datatable-programming.Rmd

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>

* Update vignettes/datatable-reshape.Rmd

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>

* One last batch of fine-tuning

---------

Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>
Co-authored-by: Michael Chirico <chiricom@google.com>
@sritchie73 @MichaelChirico @ben-schwen
Improved handling of list columns with NULL entries (#4250)
33de641 
* 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
clarify that list input->unnamed list output (#5383)
e2d0a9c 
* clarify that list input->unnamed list output

* Add example where make.names is used

* mention role of make.names
@MichaelChirico
fix subsetting issue in split.data.table (#5368)
c42e787 
* fix subsetting issue in split.data.table

* add a test

* drop=FALSE on inner [
@MichaelChirico
switch to 3.2.0 R dep (#5905)
0071ff4
@MichaelChirico
Allow early exit from check for eval/evalq in cedta (#5660)
2dfa42b 
* Allow early exit from check for eval/evalq in cedta

Done in the browser+untested, please take a second look :)

* Use %chin%

* nocov new code
MichaelChirico and others added 2 commits February 17, 2024 01:11
@MichaelChirico
Friendlier error in assignment with trailing comma (#5467)
358f2bf 
* friendlier error in assignment with trailing comma

e.g. `DT[, `:=`(a = 1, b = 2,)`.

WIP. Need to add tests and such, but editing from browser before I forget.

* Another pass

* include unnamed indices on RHS too

* tests

* NEWS

* test numbering

* explicit example in NEWS
@MLopez-Ibanez @MichaelChirico
Link to ?read.delim in ?fread to give a closer analogue of expected b…
8e2cc18 
…ehavior (#5635)

* fread is similar to read.delim (#5634)

* Use ?read.csv / ?read.delim

---------

Co-authored-by: Michael Chirico <chiricom@google.com>
Co-authored-by: Michael Chirico <michaelchirico4@gmail.com>
@MichaelChirico
Copy link
Member

OK, current state LGTM. I'll leave it to someone else to merge (again, not squash!)

@tdhock
Copy link
Member Author

tdhock commented Feb 18, 2024

not sure what is the right way to do that...
"Create a merge commit" or "Rebase and merge" ? What is the difference?

@MichaelChirico MichaelChirico merged commit 4e274ab into master Feb 18, 2024
5 checks passed
@MichaelChirico MichaelChirico deleted the 1-15-99 branch February 18, 2024 18:40
@MichaelChirico
Copy link
Member

Not 100% sure, but I went with "Rebase and merge". Seems to have worked -- I believe the former would have added a new commit like "merge branch 1-15-99 to master", which I don't think we want in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaelChirico MichaelChirico MichaelChirico approved these changes

@jangorecki jangorecki Awaiting requested review from jangorecki jangorecki is a code owner

@mattdowle mattdowle Awaiting requested review from mattdowle

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@tdhock @MichaelChirico @jangorecki @OfekShilon @Anirban166 @davidbudzynski @sritchie73 @MLopez-Ibanez