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

[RFC] Re-support of original X11 color names #386

Closed
kimikage opened this issue Dec 19, 2019 · 8 comments
Closed

[RFC] Re-support of original X11 color names #386

kimikage opened this issue Dec 19, 2019 · 8 comments

Comments

@kimikage
Copy link
Collaborator

kimikage commented Dec 19, 2019

I ended support for parsing some X11 color names which conflict with the SVG/CSS specification (e.g. "deep sky blue") in PR #360. However, some users or packages use them.
cf. GiovineItalia/Gadfly.jl#1368

I don't think it's a good idea to use the old synonyms in modern times because it goes against the intention of standardization (formalization) of the SVG/CSS color.
Moreover, in the past, it seems that there is a case where the aliases with spaces have been deleted.
https://web.archive.org/web/20070928030522/http://cvsweb.xfree86.org/cvsweb/xc/programs/rgb/rgb.txt.diff?r1=1.2&r2=1.1

However, I would not object to someone adding them to color_names.
I'm against the removal (neglect) of all spaces. (e.g. "p ink"->"pink")

@kimikage kimikage changed the title Re-support of original X11 color names [RFC] Re-support of original X11 color names Dec 19, 2019
@timholy
Copy link
Member

timholy commented Dec 20, 2019

Sorry I failed to notice this had changed when I reviewed #360. The kindest option is to issue a deprecation warning. If you are ambitious you can do that with a 0.10.1 and 0.11.1 release (you'll need to create some branches and use git-cherry-pick). I won't have time to help due to travel, unfortunately.

@kimikage
Copy link
Collaborator Author

I'm sorry, but I didn't think this would be a problem. I was wrong.

I think the deprecation warning is useful only when there are alternatives. For the users who want the traditional names with spaces, the names without spaces should not be alternatives.

Unfortunately, I realized that #374, which designed in anticipation of the parser changes (cf. #359 (comment)), was not so useful.:sob:

@kimikage
Copy link
Collaborator Author

kimikage commented Dec 30, 2019

Perhaps we have four choices.

  1. Add the deprecation warning to color parser and release v0.10.1 and v0.11.1
  2. Assuming PR Modify strictness of color parser (#357) #360 is a bug, merge PR [RFC] Add support for parsing X11 color names with spaces #388(-like something) and issue v0.10.1 and v0.11.1
  3. As a feature extension, merge PR [RFC] Add support for parsing X11 color names with spaces #388(-like something) and release v0.12.0 with other changes (e.g. Add support for specifying the style of hex #387, [RFC] Add support for parsing 8-digit and 4-digit hex notations #371)
  4. Do nothing about this and release v0.12.0 with other changes

I prefer to re-support the traditional colors rather than add warnings. However, we need to be desperately aware that when we support somethings, it will be awkward to break them.

Edit:
To be honest, although I follow the consensus of the communities, I cannot really agree with the release of v1.0 (i.e. the Semantic Versioning) after watching the recent CompatHelper festival.

@johnnychen94
Copy link
Member

johnnychen94 commented Dec 30, 2019

Generally, option 1/2 looks more reasonable to me, but since I don't use this package directly, this is just some random jump-in comment.

I cannot really agree with the release of v1.0 (i.e. the Semantic Versioning) after watching the recent CompatHelper festival.

I think the core idea here is to let non-breaking changes(i.e., patch version) pass through the compat limitation silently and for every breaking changes manually check if repositories are affected. This was a heavy workload for the maintainers before the availability of CompatHelper so we didn't adhere to this "principle". But since CompatHelper watches the changes and raises a notification to all repositories that might be affected by that, it becomes possible to take this "principle" into practice even if it's only a few of us now.

Yes, it's some kind of annoying. But what we can expect is when codes become stable, there will be less breaking changes at the root of the dependency chain (e.g., ColorTypes.jl). And it's useful in practice, for example, JuliaImages/ImageCore.jl#107 absorbs the depwarn raised by ColorTypes so that downstream packages won't get the warning.

Of course, this "explanation" is also very personal.

@kimikage
Copy link
Collaborator Author

I think all options are valid since Colors.jl is v0. However, if Colors.jl was v1.0 or later, we would have to bump the major version up.
Some people don't think adding color names is a breaking change, but it definitely changes the behavior of parser. As a matter of fact, regarding the color names with spaces, I do not explicitly violate any docs or tests in Colors.jl with PR #360. And this is what happened.

My concern is already in the FAQ.
https://semver.org/#doesnt-this-discourage-rapid-development-and-fast-iteration
https://semver.org/#if-even-the-tiniest-backwards-incompatible-changes-to-the-public-api-require-a-major-version-bump-wont-i-end-up-at-version-4200-very-rapidly

@johnnychen94
Copy link
Member

johnnychen94 commented Dec 30, 2019

As a matter of fact, regarding the color names with spaces, I do not explicitly violate any docs or tests in Colors.jl with PR #360

Yes, this can be interpreted as a bug or just some undefined behaviors; people treat this differently -- some fixes it and some don't. If you're blaming yourself on making this "mistake", which is not, then please don't. What's more important in "rapid development" in my own view is that the overall quality and functionality of the codebase becomes better with your efforts in. Since you're one of the main maintainers at present, we have great trust in your decision <-- someone said this to me when I was new to this community :D The fact is nobody pays you to write codes here, and we're contributing under MIT license, so... just don't be so nervous about "bug reports".

As I said, my comment can be regarded as some random noise, it's more important that you make your choice with your full consciousness and you take the responsibility of any following consequences(be proud of it or make more efforts to it).

Well, it's off-topic, but I hope this makes you feel better ❤️ It's a long journey and take it easy.

@timholy
Copy link
Member

timholy commented Dec 30, 2019

I prefer your first choice, "Add the deprecation warning to color parser and release v0.10.1 and v0.11.1". I don't really like #388. Can't we just strip spaces from the string, while issuing a warning?

One general comment: IMO (and it's just an opinion) it's better to fix unintended breakages fairly quickly, so it would have been best if you'd just made a decision & run with it than to wait a couple weeks for input from others.

@kimikage
Copy link
Collaborator Author

kimikage commented Dec 30, 2019

Can't we just strip spaces from the string, while issuing a warning?

We cannot do it strictly. Let's make it loose. 😄

Edit:
I noticed that not throwing the errors would block colorant_str in NamedColors.jl.
https://github.com/JuliaGraphics/NamedColors.jl/blob/d3d62e3b41b8ebc842b4864e032f4f4078b02956/src/namelookup.jl#L61-L70

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

No branches or pull requests

3 participants