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

MakeColor now indicates conversion errors by returning a second value #27

Merged
merged 3 commits into from
May 26, 2018

Conversation

muesli
Copy link
Contributor

@muesli muesli commented May 23, 2018

MakeColor now returns a bool as its secondary return value, indicating
whether the conversion failed because the source color's alpha channel
was set to 0. In such cases, the returned color will have R, G and B
set to 0, too.

Note: this will break the API. I'm not sure how many projects depend on go-colorful, but it might be a good idea to package the current state as a proper release before merging this.

Fixes #21.

MakeColor now returns a bool as its secondary return value, indicating
whether the conversion failed because the source color's alpha channel
was set to 0. In such cases, the returned color will have R, G and B
set to 0, too.
@lucasb-eyer
Copy link
Owner

Thanks, very nice PR.

I believe quite a few projects depend on go-colorful, so I agree with your suggestion. Since I have never packaged anything for Go, could you point me to the docs on The Right Way™ to do this? Then I will mark the current state as 1.0, merge, and mark the post-merge one as 1.1 (or do you have other suggestions/are the conventions different in golang?)

Oh and we could keep the Q/A in the README if we reformulate the question as "Q: Why would `MakeColor` ever fail!?" and the answer can stay as-is. This way, we both (1) keep the link to the interesting discussion in the issue in the README, and (2) give its own little section to this, as most people just skim the README, then wonder later on.

@muesli
Copy link
Contributor Author

muesli commented May 25, 2018

Happy to help, of course!

The general convention and my recommendation would be following semver. This means go-colorful would be compatible with dep's and vgo's version selection algorithms, too.

Sticking to semver, you're guaranteeing API compatibility for releases of the same major version:

Given a version number MAJOR.MINOR.PATCH, increment the:

    MAJOR version when you make incompatible API changes,
    MINOR version when you add functionality in a backwards-compatible manner, and
    PATCH version when you make backwards-compatible bug fixes.

That in turn means the current codebase should either be releases as v0.x (where x is presumably either 1 or 9) or as the official v1.0 release. Everything after this PR gets merged would need to be released with an incremented major version, though! (v1.x or v2.x)

I'll amend the README as you suggested. We should probably also mention the breaking API change between the two release there.

NB: Really include the v in the version name / git tag (releasing on GitHub even creates the tag for you!), so both dep and vgo behave nicely.

@lucasb-eyer
Copy link
Owner

Thanks! I'm aware of semver but didn't know the tools actually make use of it, good to know. I'd go for v0.9 with the current one and v1.0 after the PR then.

What I'm wondering about is how to actually "make a release" in the Go world? I've done so in Py and C++ before, but never Go. Is it only creating a release/tag on Github, or is there other things like some files in the repo? Thanks for your help already and sorry for not searching the info myself, I'm lazy on travel 😄

Good point on mentioning the change in the README. I would add a Release Notes section in the README after Who? / before License: MIT with content as at the end of this message. I can do that when creating the release.

By the way, feel free to add your name at the end of the README in the Who? section.


Release Notes

Version 1.0

  • API Breaking change in MakeColor: instead of panicing when alpha is zero, it now returns a secondary, boolean return value indicating success. See [TODO, link to README section] for details.

Version 0.9

  • Initial version number after having ignored versioning for a long time :)

@muesli
Copy link
Contributor Author

muesli commented May 26, 2018

You indeed only need to go to the Releases tab of your project on GitHub and proceed to create a new release there. It will create the git tag for you (e.g. v0.9), as well as downloadable tar-balls, and... that's it, you're all set 😄

@muesli
Copy link
Contributor Author

muesli commented May 26, 2018

@lucasb-eyer Done and done. As discussed, I haven't included the Release Notes paragraph in the README yet. Merging this PR and adding the release notes to the README should be done after releasing v0.9, obviously.

@lucasb-eyer lucasb-eyer merged commit 5cff8b2 into lucasb-eyer:master May 26, 2018
@lucasb-eyer
Copy link
Owner

And done, and done, and done 😄 These were some very welcome contributions, and thank you for having the patience of explaining things to me!

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.

2 participants