Skip to content

Add last grey/gray spelling combination #4407

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

Closed
wants to merge 1 commit into from

Conversation

thomasp85
Copy link
Member

@thomasp85 thomasp85 commented Apr 9, 2021

This is not really the best, and I'm open to alternatives.

ggplot2 originally had scale_colour_grey(), and scale_color_grey(), with the former mixing up English/American spelling of colour and grey. A recent PR added scale_color_gray() which makes the tests fail as it looks for whether all English colour scales have an American equivalent. This PR adds scale_colour_gray() which fixes the test, but leaves a sour taste in my mouth. Dream scenario would be to only have scale_colour_grey() and scale_color_gray() as those names would be internally consistent, but I don't think we can remove the original mixed one as it would cause major breakage...

The alternative is to change the tests to remove one of the versions but that seems almost just as bad...

Thoughts

@hadley
Copy link
Member

hadley commented Apr 9, 2021

Why don't we just remove scale_color_gray()?

@clauswilke
Copy link
Member

Counter-argument: I can never remember whether it's grey or gray and use them interchangingly.

@yutannihilation
Copy link
Member

I too can't remember it, but always use tab completion so it's better if there are fewer candidates. (But I have no strong opinion here)

Copy link
Member

@clauswilke clauswilke left a comment

Choose a reason for hiding this comment

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

I'm approving this but don't have a strong opinion one way or another.

@thomasp85
Copy link
Member Author

@hadley, it was recently added because it was lacking and US spelling users requested it

@yutannihilation
Copy link
Member

@hadley
A bit more context is that you indicated that adding scale_color_gray is "a good idea," which aren't implemented just because we don't have the development bandwidth. This was the reasoning of the PR (#4299).

This seems like a good idea, but unfortunately it's not high enough on my priority list that I will implement it myself. However, I'd be happy to review a PR.
#1901 (comment)

@hadley
Copy link
Member

hadley commented Apr 9, 2021

@yutannihilation that's just my standard response. I wouldn't read anything into it.

I think reverting #4299 is the simplest fix — scale_color_gray() (no matter how you spell it) is relatively rarely used, and the spelling difference is small enough that I don't think it's really causing any problems.

@yutannihilation
Copy link
Member

I see... Yeah, I personally agree with reverting it.

@clauswilke
Copy link
Member

Fine with reverting also. Maybe I'll finally learn the correct spelling. :-)

@hadley
Copy link
Member

hadley commented Apr 9, 2021

Its A for America (grAy) and E for England (grEy) 😄

@clauswilke
Copy link
Member

And what do they use in Australia?

@thomasp85
Copy link
Member Author

ok, I'll revert instead

@thomasp85 thomasp85 closed this Apr 12, 2021
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.

4 participants