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

Cannot pass empty grouping_mark to locale() #26

Closed
peterdesmet opened this issue Aug 20, 2021 · 7 comments
Closed

Cannot pass empty grouping_mark to locale() #26

peterdesmet opened this issue Aug 20, 2021 · 7 comments
Assignees
Labels
bug Something isn't working dependency Caused by or related to a dependency function:read_resource Function read_resource()
Milestone

Comments

@peterdesmet
Copy link
Member

peterdesmet commented Aug 20, 2021

Since the upgrade to readr 2.0.0 (or maybe other packages), the integer and number tests fail, specifically on the property bareNumber:

https://github.com/inbo/datapackage/blob/a195f686a86b46b6042966db69c3cc5ee2c7c237/tests/testthat/types.json#L70

The moment that property is added in the JSON file (whether true or false) it stalls any reading of the file. I haven't figured out why. bareNumber is handled here:

https://github.com/inbo/datapackage/blob/a195f686a86b46b6042966db69c3cc5ee2c7c237/R/read_resource.R#L337

@peterdesmet peterdesmet added the bug Something isn't working label Aug 26, 2021
@peterdesmet peterdesmet changed the title Fix test bugs introduced by upgrading to readr 2.x bareNumber test crashes R Aug 27, 2021
@peterdesmet
Copy link
Member Author

The issue is caused by passing an empty grouping_mark to locale, which is used by col_number().

@peterdesmet peterdesmet removed the readr label Sep 7, 2021
@peterdesmet peterdesmet added this to the readr milestone Sep 7, 2021
@peterdesmet
Copy link
Member Author

This is resolved by passing ♥ as grouping_mark: a non-empty, unlikely to occur value, see 26e7482:

This value is set to "♥" because it is unlikely to occur and will only be used for bareNumber=false with undefined grouping_mark. It is not set to "" because tidyverse/readr#1241 nor "," because that can conflict with non-default decimal_mark.

@damianooldoni please provide feedback? Should we revisit this when readr allows "" as grouping_mark again? Is that likely to happen?

@peterdesmet
Copy link
Member Author

Ok, I'm getting:

Found the following file with non-ASCII characters:
    read_resource.R
  Portable packages must use only ASCII characters in their R code,
  except perhaps in comments.
  Use \uxxxx escapes for other characters.

Suggestions for other characters?

@damianooldoni
Copy link
Contributor

I give it a look now

@damianooldoni
Copy link
Contributor

I agree to give a non usual character for grouping mark, but still I would choose it within the characters you can find on a normak keyboard 😄
Why not setting it up to "~" for example? The usage of this character in a character column will never create any problem, as the grouping mark is only searched in numeric columns. So, we shouldn't go too exotic. Or am I missing something?

And what if a user specifies "~" as decimal mark, but no grouping mark? Then set "," as grouping mark instead, I would say.

@peterdesmet
Copy link
Member Author

peterdesmet commented Sep 8, 2021

Thanks, I have opted for ¡ (upside-down !, ascii 173, 18023da), since ~ is more likely to occur in number fields as "approximately". I did not add code in case the user sets ¡ as decimal mark, as I rather have it display the default readr error:

Error: decimal_mark and grouping_mark must be different

Setting , as default (not user explicit) grouping mark can lead to unreported issues in barenumber=false.

@peterdesmet peterdesmet changed the title bareNumber test crashes R Cannot pass empty grouping_mark to locale() Sep 8, 2021
@peterdesmet
Copy link
Member Author

Correction, that was also extended ASCII. Now opted for backtick. Ideally, this should be reverted back to "" once readr allows it again.

@peterdesmet peterdesmet added function:read_resource Function read_resource() dependency Caused by or related to a dependency labels Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dependency Caused by or related to a dependency function:read_resource Function read_resource()
Projects
None yet
Development

No branches or pull requests

2 participants