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

suggested changes to allocate #182

Open
dylanbeaudette opened this issue Jan 14, 2021 · 4 comments
Open

suggested changes to allocate #182

dylanbeaudette opened this issue Jan 14, 2021 · 4 comments
Assignees

Comments

@dylanbeaudette
Copy link
Member

dylanbeaudette commented Jan 14, 2021

I can't figure out how to document all of the arguments to allocate such that arguments specific to sub-functions are clear in the manual page.

Ideas:

  • application of droplevels should probably happen after sub-functions are done, fewer arguments to pass around
  • sub-functions should be exported with their own documentation, manual page for allocate is generic with links
  • consider splitting fao_lev into two vectors (salinity and sodicity), matching by absolute position within fao_lev is tricky and breaks if classes change
  • consider optional output with saline / sodic classes in their own columns (result is a matrix or data.frame) so that cross-classification is possible e.g. "slightly saline / moderately sodic".
  • allocate(EC = NA, pH = 5, ESP = 1, to = 'FAO Salt Severity', droplevels = FALSE) results in an error
@dylanbeaudette
Copy link
Member Author

Function details moved to Details section for now.

dylanbeaudette added a commit that referenced this issue Jan 19, 2021
@brownag
Copy link
Member

brownag commented Jan 20, 2021

I intend to open a PR linked to this issue with my suggestions and comments in it. I feel this will be easier to provide specific feedback on instances/lines of code. I am not going to do that before 1.27 goes to CRAN though -- and probably not before upcoming stats class -- so as to not detract from anything we need to do before that.

@brownag
Copy link
Member

brownag commented Jan 20, 2021

Good work on fixing it up for this release though #179

@dylanbeaudette
Copy link
Member Author

Hi @smroecker any thoughts on the above commentary / ideas for allocate? This won't prevent aqp 2.0 from being released, but now is a convenient time to make large changes.

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

No branches or pull requests

3 participants