Closed szymanskir closed 4 years ago
Thanks @szymanskir - that's a good catch and looks like a great fix. I'll check it out early next week, and expect to merge then. Thank you for using the package, and for your input here
@szymanskir all looks good, but could you please move the new line to a single instance outside the if
clause, and so have just a single line to replace all "NULL"
with empty strings? Could you maybe also add a comment referencing issue#96, and explaining that these "NULL"
values arise only in NYC data from Aug 2018 onwards? Happy to merge once that has been done.
Note also that the travis fail is not related to your PR, so you may ignore that (it's related to #95)
One more thought: The boost::replace_all
call comes with a time cost, so it'd probably also be better to only run it where needed by first having an extra line of if (utils::strfound (city, "ny"))
- this is equivalent to if (city == "ny")
, but string equality in C++ doesn't work like that, so these comparisons use a safer, locally-defined strfound
function.
@mpadge thank you for the review. I have modified the code according to your remarks.
One more thing @szymanskir: We aim to acknowledge all contributions and all contributors here, so would you please be so kind as to add your name to the DESCRIPTION
file with role = "ctb"
(after Tom Buckley)? If possible, could you also run codemetar::write_codemeta()
to update the metadata file? Thanks!
This is a patch regarding #96 . I looked around the code and noticed the part where
\N
(the previous way how missing values where marked in the nyc data) characters are removed. I added lines removing "NULL" and this seems to be fixing the issue :D