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

In calculate, should F and Chisq stats be lower case? #373

Closed
davidhodge931 opened this issue Apr 2, 2021 · 6 comments
Closed

In calculate, should F and Chisq stats be lower case? #373

davidhodge931 opened this issue Apr 2, 2021 · 6 comments

Comments

@davidhodge931
Copy link

Unless there is some reason I'm unaware of, I think it would be easier to remember if all stats were lower case, i.e. if F & Chisq were f and chisq

@davidhodge931 davidhodge931 changed the title In calculate, it'd be good if all stats were lower case In calculate, it'd be good if all stats were lower case Apr 2, 2021
@davidhodge931 davidhodge931 changed the title In calculate, it'd be good if all stats were lower case In calculate, should F and Chisq stats be lower case? Apr 2, 2021
@simonpcouch
Copy link
Collaborator

#37 (comment) might be relevant here.

One solution here could be to check if the inputted stat matches one of the options tolower()ed--in that case, maybe:

a) message that chisq/f was replaced with Chisq/F and move forward? Or...
b) continue to error out but do so more informatively? This approach feels more tidy, I think.

echasnovski added a commit that referenced this issue Apr 12, 2021
@echasnovski
Copy link
Collaborator

I have the same reasoning. If designing from beginning I'd make them all lowercase, but now making them lowercase will break existing code. Allow case insensitive matching seems too permissive.

I added a "standard" tidyverse way of matching argument with a predefined set: it will error if argument is not in the set and suggest an alternative in case of a "not very big" typo. Currently this doesn't work with single letter values, but might be fixed in the future.

@ismayc, do you have any strong opinions here?

@ismayc
Copy link
Collaborator

ismayc commented Apr 12, 2021

I agree with Simon's suggestion. Provide a message but allow either case to work

@ismayc
Copy link
Collaborator

ismayc commented Apr 12, 2021

F and Chisq were chosen because capital letters were common in historical examples.

echasnovski added a commit to echasnovski/infer that referenced this issue Apr 13, 2021
@echasnovski
Copy link
Collaborator

echasnovski commented Apr 13, 2021

Thanks for suggestion. Implemented both current and lower case via allowed aliases.

@github-actions
Copy link

This issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants