Skip to content

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