Skip to content

Address a few bugs #74

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

Merged
merged 7 commits into from
Mar 18, 2023
Merged

Address a few bugs #74

merged 7 commits into from
Mar 18, 2023

Conversation

dshemetov
Copy link
Contributor

@dshemetov dshemetov commented Mar 15, 2023

Closes #69.
Closes #66.
Closes #65.
Closes #67.

On the last issue: request params get formatted differently depending on whether a vector or a list is handed as an argument. This is an attempt to make the outcome uniform. The issue is

r$> paste(sapply(c("290", "190"), toString), collapse = ",")
[1] "290,190"

r$> paste(sapply(list(c("290", "190")), toString), collapse = ",")
[1] "290, 190"

So I added some logic to check for vectors and use as.list instead. The problem is that the is.vector also yields TRUE for lists, so it's currently messing up the list-type handling. Would appreciate help/commits there.

@brookslogan
Copy link
Contributor

The function you wanted was probably is.atomic. But I wonder if we can keep the logic a bit simpler, back out your change, and instead fix toString to be paste, collapse=",". And maybe also use vapply instead of sapply (FUN.VALUE would be character(1L)).

* Revert back to old `format_list` logic, which was simpler
* Fix `format_item` to not insert extra spaces after some commas, which resulted
  in incorrect API requests.
* Use `vapply` rather than `sapply` in `format_list` to potentially catch weird
  cases and express expectations.
* Add a few formatting tests.
Copy link
Contributor

@brookslogan brookslogan left a comment

Choose a reason for hiding this comment

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

Looks good, though hope we don't depend on int zips/fips somewhere already. Committed the approach I described above. Do you remember what request revealed the fip_code typo? Wonder if we're missing some tests.

@brookslogan brookslogan self-requested a review March 18, 2023 02:16
@dshemetov
Copy link
Contributor Author

Thanks Logan! I don't recall what revealed the fip_code typo. To say that we're missing tests is an understatement: those two functions format_list and format_item are the only functions that have any tests in this repo at all.

Just going to merge and leave making unit tests to another PR.

@dshemetov dshemetov merged commit 6ee7261 into dev Mar 18, 2023
@dshemetov dshemetov deleted the ds/bug-fix branch March 18, 2023 03:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants