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

Do not include symbol on lhs of := in othervars #2329

Merged
merged 4 commits into from
Sep 6, 2017

Conversation

renkun-ken
Copy link
Member

@renkun-ken renkun-ken commented Sep 5, 2017

This is an attempt to fix #2326.

@renkun-ken renkun-ken changed the title Do not include symbol on lhs of := in othervars (Fix #2326) Do not include symbol on lhs of := in othervars Sep 5, 2017
@MichaelChirico
Copy link
Member

Hi Kun, thanks!

Please be sure to include tests and make sure the updated package compiles and that test.data.table() runs without error -- in particular I think it's likely there's a reason the LHS was included in othervars, which should be covered by another test, so this bug fix will have to allow that use case to continue to function.

See here:
https://github.com/Rdatatable/data.table/blob/master/Contributing.md

@mattdowle
Copy link
Member

Hi Kun,
Thanks from me too. In case you are on Ubuntu, I recently added my dev cycle in the cc.R file in the project root. That's how to easily make changes and run all tests, at least how I do it. Once that is passing you can do the longer R CMD build and R CMD check before pushing the pull request. If you are on Mac or Windows then cc.R will likely need some tweaking.
Hope that helps,
Matt

@renkun-ken
Copy link
Member Author

Thanks for reminding! I'm working on macOS at the moment (or may turn to Ubuntu if necessary) and will need a little while to get familiar with data.table's testing and find out what breaks previous test cases.

@codecov-io
Copy link

codecov-io commented Sep 6, 2017

Codecov Report

Merging #2329 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2329      +/-   ##
==========================================
+ Coverage   90.92%   90.96%   +0.03%     
==========================================
  Files          61       61              
  Lines       11800    11804       +4     
==========================================
+ Hits        10729    10737       +8     
+ Misses       1071     1067       -4
Impacted Files Coverage Δ
R/data.table.R 97.42% <100%> (ø) ⬆️
src/forder.c 94.47% <0%> (+0.52%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a869907...9758cf9. Read the comment docs.

@mattdowle mattdowle merged commit c98142c into Rdatatable:master Sep 6, 2017
@mattdowle mattdowle added this to the v1.10.6 milestone Sep 6, 2017
@mattdowle
Copy link
Member

Great PR!! Thanks!

@MichaelChirico
Copy link
Member

Nice @renkun-ken! The PR process can be a steep learning curve, glad you could navigate it 🤓

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.

.SD mistakenly includes column being set when get() appears in j
4 participants