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

Don't warn about numeric coercion in set(j=) #6595

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MichaelChirico
Copy link
Member

Closes #6594

@MichaelChirico
Copy link
Member Author

Should we also cover the i= case here?

i is slightly more likely to have benefits from avoiding (e.g. 1e8 rows), but I think the same argument still basically applies.

If you received row_idx from out of your control (e.g. as output from another function), you'll still need to run as.integer() to overcome this warning. A wash.

@MichaelChirico
Copy link
Member Author

MichaelChirico commented Oct 31, 2024

NB: the rchk error is unrelated to the current PR, probably an upstream change (though rchk has no recent changes, maybe the docker image was updated recently, or maybe there's a new LLVM version catching new issues):

Here's a PR run without this PR applied showing the same error; the change happened in the last 5 days since that commit to master shows clean.

https://github.com/Rdatatable/data.table/actions/runs/11616342257/job/32349061905

@ben-schwen
Copy link
Member

I would either change both i and j or none of them. I agree that it's strange that we warn about this, but having a verbose message seems appropriate. Educating ppl if they are eager to learn 😁

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.

Don't warn about coercing j to integer
2 participants