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

Do not export internal helper functions #56

Closed
florianm opened this issue Apr 16, 2020 · 1 comment
Closed

Do not export internal helper functions #56

florianm opened this issue Apr 16, 2020 · 1 comment
Assignees
Milestone

Comments

@florianm
Copy link
Collaborator

Feature

Source ropensci/software-review#335 (comment)

I think your idea of dropping the default argument for col_contains makes sense. Also, do you see any reason for ru_datetime or isodt_to_local to be exported now that they are only used internally?
I see that there are a few places in the documentation where ru_datetime is mentioned. In the details for form_schema_parse:
"form_schema_parse recursively unpacks the form and extracts the name and type of each field. This information then can be used to inform the user which columns require ru_datetime, attachment_get, or attachment_link, respectively"
And I see that ru_datatime is used in the example for attachment_link.
If you decide not to export the functions that you are using only internally (totally your choice), then I think you should edit the documentation so that it does not point users to them. If you do export them, and use cases are not demonstrated in the vignettes, then they need to be well documented. Just take an extra moment to make sure that you are saying all you want to in their help.
These same points, exporting internal functions and mentioning internal functions in the documentation, should be considered for the other functions you wrote handlers for: split_geopoint and attachment_get.

  • Internal helpers: ru_datetime, isodt_to_local, split_geopoint, attachment_get
  • Extend documentation for internal helpers with "how to do it by hand" examples
  • If not exporting, remove mentions in documentation?
  • If exporting, explain clearly in documentation (maybe separate vignette "under the bonnet")?
@florianm florianm added this to the rOpenSci milestone Apr 16, 2020
@florianm florianm self-assigned this Apr 16, 2020
@florianm
Copy link
Collaborator Author

florianm commented Apr 28, 2020

I think your idea of dropping the default argument for col_contains makes sense.

The default argument is now superseded by form schema introspection, therefore happy to drop.

do you see any reason for ru_datetime or isodt_to_local to be exported now that they are only used internally?

Removed export, turned into internal: simple parsing helpers: ru_datetime, isodt_to_local
Deleted: ru_datetime - handle_ru_datetimes now uses isodt_to_local directly
Still exported and well (enough?) documented in examples and vignettes: functions that are core to ruODK's parsing workflow: split_geopoint, attachment_get

I see that there are a few places in the documentation where ru_datetime is mentioned. In the details for form_schema_parse: <...>

Less of an issues now: form_schema_parse is only used in legacy mode (ODK Central version < 0.8)

And I see that ru_datetime is used in the example for attachment_link.

Not any more! attachment_link now uses handle_ru_datetimes in examples - thanks for the prompt

If you decide not to export the functions that you are using only internally (totally your choice), then I think you should edit the documentation so that it does not point users to them. If you do export them, and use cases are not demonstrated in the vignettes, then they need to be well documented.

  • Internal: do not mention in any of man / vignette / readme - done for
    • ru_datetime
    • isodt_to_local
    • strip_uuid
    • prepend_uuid
    • attachment_url
    • unnest_all
  • Export: show ex / demo in vignette / possibly mention in readme:
    • split_geopoint has an example to show order of lon / lat / alt (coordinate order is notorious for swap errors vs. geojson)
    • attachment_get is discussed in vignette odata-api
    • Several functions which are still relevant for transparency have been moved to "utilities". This provides context while prioritising the main functions under the "OData API" / "RESTful API" reference topics.

Thanks to Jason for prompting a good clean-up of the docs and code!

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

1 participant