Parameterize key query fields in riem_measures() #48
Conversation
…t values for station, date_start args (thus making them required); Convert elev and latlon to logicals; Update DESCRIPTION and documentation
|
Ready for review, pending my outstanding question on default value for |
|
I'm unsure what your standard practice is, but I'll leave it to you to click "Resolve conversation" on any items resolved to your liking. Thanks! |
|
@JElchison Thank you!! I made a few changes
I have two requests for you:
Afterwards I'll update the NEWS file and will plan a new release in January. Thanks again! |
Yes, they look good! I asked a couple clarifying questions as comments.
Yes, will do! |
tests/testthat/test-measures.R
Outdated
| date_end = "2013-12-01")) | ||
| }) | ||
|
|
||
| # TODO validate 'data' against [all, tmpf, dwpf, relh, drct, sknt, p01i, alti, mslp, vsby, gust, skyc1, skyc2, skyc3, skyc4, skyl1, skyl2, skyl3, skyl4, wxcodes, ice_accretion_1hr, ice_accretion_3hr, ice_accretion_6hr, peak_wind_gust, peak_wind_drct, peak_wind_time, feel, metar, snowdepth] # nolint: line_length_linter |
There was a problem hiding this comment.
should we tackle this in this PR?
There was a problem hiding this comment.
Short answer: I don't think so.
Longer answer: I left this TODO here as a placeholder while I ponder the merits of such a check. The more I think about it, the less I'm convinced of its value. data is the closest thing we have to a pass-through parameter. Since the only value our code counts in is all, and since we already test that case with a snapshot, I'm not sure what additional benefit that adding this check would provide. Certainly it would increase our dependence on IEM's data structure, as this test would need updated if IEM ever added, renamed, or removed any fields. Unless you object, I'm in favor to remove the TODO and instead rely on the existing snapshot to test what we care about.
|
@JElchison Thank you! Are you happy with my own changes to the checks on the data arguments? I'll come back to this PR in the new year, merge it and work on a release (I might also tackle the other breaking change at the same time). Thank you and happy holidays! |
…ssociated test); Add/update comments for clarity
R/measures.R
Outdated
| #' @param report_type A vector of strings, representing report types to query. | ||
| #' The available options are `"hfmetar"`, `"routine"`, `"specials"`. | ||
| #' Default value is `c("routine", "specials")`. | ||
| #' The report is then in the `metar` column of the output. |
There was a problem hiding this comment.
The report is then in the
metarcolumn of the output.
FYI I'm removing this comment since it's untrue. A "report" in this context is a single line of reported weather data. Each report (line) is one of the 3 available report types. It's true that IEM modifies the METAR column in the case of hfmetar, but no observable changes are made in the METAR column for either routine or specials.
I sure am, thanks! I've made a few final changes of my own ... mainly documentation, but also one fix ( It looks like we are down to 2 "unresolved" conversations. If you're okay with their outcomes, then I'm good to button this up for merging in the new year. Happy holidays to you as well! |
|
Happy New Year @JElchison! Thanks again for this PR! |
Expose key query fields in
riem_measures()as parameters, including:data(defaults toallas before)elev(defaults tonoas before)latlon(defaults toyesas before)report_type(new default is3,4, which skips HFMETAR)Update
date_startto have safer default (2024 instead of 2014). Specify missingnometaquery field. Update comments, self-documentation, and lintr exceptions accordingly.