search

tidyverse / vroom

Fast reading of delimited files
https://vroom.r-lib.org
Other
621 stars 60 forks source link

Use .empty() method #472

Closed MichaelChirico closed 1 year ago

MichaelChirico commented 2 years ago

https://releases.llvm.org/5.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/readability-container-size-empty.html

jennybc commented 1 year ago

I'm happy to merge (and will do so). Is there more context? Such as: does this come from running some automatic tool? I assume so and yet it's also a bit surprising that this is the only affected locus.

MichaelChirico commented 1 year ago

Hi Jenny, definitely appreciate your concerns about trying to prevent this issue from recurring.

This was flagged by our internal build tools; I'm not sure if there's an open-source equivalent.

Yes, this is the only place affected in this version of vroom. I've filed several similar patches to other projects:

https://github.com/pulls?q=is%3Apr+author%3AMichaelChirico+archived%3Afalse+.empty

I'm only passively looking for the issues -- it is flagged when filing a "PR" to update the package, and only takes a few seconds to file a PR in the GitHub UI. I would have to look into a way to flag the issue more proactively to catch all uses in (our versioned copies of a subset of) R packages.

Searching around for "readability-container-size-empty" does yield some open-source stuff, e.g. from microsoft:

https://github.com/microsoft/clang-tools-extra/blob/master/test/clang-tidy/readability-container-size-empty.cpp