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

New argument/error checking approach, new features, addressing #158, #223, & #262 #263

Merged
merged 18 commits into from
Jun 20, 2022

Conversation

jmobrien
Copy link
Collaborator

Parents, talk to your children about the dangers of refactoring code.

Okay, so this got into a lot, but it's done now, and I think I really managed to clean things up. Quick list of what's done here:

  • All arguments are now handled in a straightforward way with functions in file arguments.R, addressing Proposal: get rid of assertthat and replace with unified argument checking/adapting framework w/rlang #262
    • General purpose functions do common checks (string, boolean, integerish, character w/o missing)
    • More detailed checks for specific arguments also perform formatting as needed
    • All error handling operates using rlang::abort (and similar), using cli-type format
    • argument checking/formatting handled directly at the start of each main function
  • Removed:
    • assertions.R (and dependency on assertthat, unless I missed something)
    • check_params(), which was redundant and no longer needed
    • Most of construct_raw_payload(), which no longer does argument formatting and just formats request bodies. This should mean that it's re-usable for other features (e.g., a function for New feature request: Deactivate surveys via API #215)
    • Some old, apparently no longer used code in the main request and response code functions.
  • New features:
    • fetch_survey() now fully handles dates & times of Date, POSIX, or equivalent character, closing ideas: setting up more flexible handling of start and end date params #158
      • Updated timezone handling
      • end_date adds 23:59:59 to time when just a date. This avoids the unintuitive situation where specifying end_date = "2020/03/03" would actually only get cases from 03/02 and before
      • default for timezone is now system locale rather than UTC, which means that things like calling `as.character() on the typical formatting of a datetime object will produce the expected results
    • fetch_survey() now has three include_* arguments that allow selection of metadata, questions, and embedded data, closing Feature: Include Embedded Data in Survey Questions #223
    • fetch_survey() now uses a more modular structure for handling the multiple calls involved in a fetch.
    • In updating the arg checking and error reporting in fetch_id(), added partial name matching & more informative error reporting when multiple surveys are matched.
  • Updated tests
    • Everything works
    • Did remove one webmockr test from test-qualtrics-api-request.R that didn't seem to work with new format for reasons I couldn't diagnose, but whose functionality seemed to be covered mostly by other tests.

@jmobrien
Copy link
Collaborator Author

One other thing I forgot to mention. In porting over the smart argument checking from #257, it now handles removing the protocol with a message, rather than completely erroring out.

@jmobrien
Copy link
Collaborator Author

Commits
be6e896 through 737f26a are to repair issues with the CMD checks. (Sorry that took a few more than I intended).

If this works for you, I think we can delete assertthat from Imports

Copy link
Collaborator

@juliasilge juliasilge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is QUITE a lot of work you did here, and what an improvement it will be for the package in the long run!! Thank you 🙌

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

Successfully merging this pull request may close these issues.

None yet

2 participants