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

PNG/JPG/GIF support: to raster or not to raster? #4564

Closed
PyvesB opened this issue Jan 23, 2020 · 15 comments · Fixed by #4756
Closed

PNG/JPG/GIF support: to raster or not to raster? #4564

PyvesB opened this issue Jan 23, 2020 · 15 comments · Fixed by #4756
Labels
npm-package Badge generation and badge templates user-poll Voice your opinion!

Comments

@PyvesB
Copy link
Member

PyvesB commented Jan 23, 2020

Users of Shields, we need your opinion!

We currently support generating PNG/JPG/GIF badges through our CLI tool. We were wondering if this feature was useful to anyone - there's not point in us maintaining and bloating our codebase with something that isn't used. We've got a PR to clean things up: #4523.

If it's a valued feature, we would look at adding it to our NPM package for feature parity. Would any of you be interested in such an option? We've got a PR to achieve this: #4524.

Simply add a reaction to this post to let us know what you think:
👍 to keep support in the CLI and add support to the NPM package.
👎 to drop support from the CLI.
👀 what, Shields has a CLI and an NPM package??

@PyvesB PyvesB added the user-poll Voice your opinion! label Jan 23, 2020
@PyvesB PyvesB pinned this issue Jan 23, 2020
@calebcartwright
Copy link
Member

👀 what, Shields has a CLI and an NPM package??

😆

@DarthRatz
Copy link

👀 what, Shields has a CLI and an NPM package??

@calebcartwright
Copy link
Member

@chris48s
Copy link
Member

Cheers for posting this.
Just to clarify, if we do drop support from the CLI, we'll continue to support serving raster badges from shields.io

@chris48s chris48s added the npm-package Badge generation and badge templates label Jan 25, 2020
@PyvesB
Copy link
Member Author

PyvesB commented Feb 1, 2020

The Twitter poll ended a couple of days ago (I set it to the maximum duration of one week), here are the results:
poll

I'm reasonably happy with the number of voters, hopefully we will get even more in future polls as we grow our community there!

The Discord poll didn't get much attention, only 3 votes since it was created, including one maintainer. I don't think there's much value in trying this one again in the future, we can just put a link to the GitHub issue in the channel.

This issue poll has had 5 distinct votes so far, including two maintainers. Not much, but it's a good centralised point for discussing the change regardless.

Overall there's a small majority in favour of dropping support (15 vs. 12). Should we keep this issue open longer? Otherwise, given that no one has voiced any strong concerns (e.g. "I absolutely really need this because X"), shall we go ahead with #4523?

@calebcartwright
Copy link
Member

calebcartwright commented Feb 2, 2020

given that no one has voiced any strong concerns (e.g. "I absolutely really need this because X")

My suggestion would be that if we decide that we want to deprecate that we give users some final period to present those arguments, as folks may or may not have realized they needed to make any arguments to accompany their votes. Perhaps another tweet along the lines of: We're currently planning to remove raster cli/lib support and will have a closing X week period to allow users to make any final arguments for keeping it

shall we go ahead with #4523

I've got even more reservations about removing raster suport from the cli after the results from the various polls/feedback mediums.

Those responses lead me to believe that there's likely a non-insignificant number of users that are leveraging the CLI capability today and/or would continue to benefit from the capability existing with the CLI and lib going forward. Though it's definitely true that individual users can indeed implement raster support via code/script, the fact that ~40% of respondants selected keep it/add it to the lib makes me think there's indeed some value (although I highly doubt anywhere near 40% of the users that download/use the gh-badges package are generating raster badges today)

I'd be more inclined to remove it from the CLI if it had been a source of bugs/maintenance overhead or if a sizeable chunk of work would be required to implement lib support/maintain support going forward (like the popout badges), but that doesn't seem to be the case.

I'd still vote for adding it to the lib (#4524), but happy to go with the flow on this one

@chris48s
Copy link
Member

Live and learn, but in retrospect, "do you want more features, or less features" maybe wasn't actually the best question.

Perhaps if we had asked something more like:

When was the last time you used the Shields CLI to generate a badge in raster format?

  • In the last month
  • In the last year
  • More than one year ago
  • Never

we might have ended up with a clearer picture.. or possibly just a differently ambiguous one.

Either way, it would be useful to pick a way forward.

The more I think about this, I'm fairly convinced that we don't actually need to regard this as a huge BC break, we can just think of it as a syntax change:

  • If you've got imagemagic installed, then if you were previously using badge build passed :green .gif, changing that to badge build passed :green | convert svg:- gif:- gives you the exact same functionality.
  • If you haven't, then badge build passed :green .gif didn't work anyway.

If we document that as a migration path, does that change your perspective at all?

@calebcartwright
Copy link
Member

The more I think about this, I'm fairly convinced that we don't actually need to regard this as a huge BC break, we can just think of it as a syntax change:

Fair enough!

If we document that as a migration path, does that change your perspective at all?

It'd help a bit yes. Again I don't feel too strongly on this, so if the consensus is for removal (and seems that's where the majority is) then we should proceed with removal. I do still think though it'd be good to do a final shoutout/RFC period like a suggested above in #4564 (comment), just for the sake of excessive transparency and communication 😄

@PyvesB
Copy link
Member Author

PyvesB commented Feb 15, 2020

@chris48s good point, you''re probably right about the survey's options. It was a good first try, but we can definitely try and make things less ambiguous next time round.

Will send round a final shoutout as suggested by @calebcartwright in the coming days!

@PyvesB
Copy link
Member Author

PyvesB commented Feb 23, 2020

Users have been informed about a two weeks closing period to share any final thoughts they may have:
https://twitter.com/Shields_io/status/1231554520842473472

Unless we've got strong views expressed in user feedback, we can merge #4523 sometime after March 8th.

@bskinn
Copy link

bskinn commented Feb 23, 2020

Caveat: I've only used the shields.io interface.

Given that perspective, though, I think @chris48s made all the argument that really needs making: if achieving the same result for rastered badges just takes passing the SVG output through imagemagick, which sounds like is already a dependency, then there's no need for the raster output in CLI or NPM.

Dropping it would make the shields tool fit better into the UNIX "do one thing well" philosophy.

@paulmelnikow
Copy link
Member

Hey @chris48s, could you summarize the decision that was made and close this out?

@PyvesB
Copy link
Member Author

PyvesB commented Apr 6, 2020

Hey @chris48s, could you summarize the decision that was made and close this out?

I believe removal from the code base is still pending #4756.

@chris48s
Copy link
Member

chris48s commented Apr 6, 2020

  • Raster support will be removed from the badges CLI tool in version 3, which is coming soon ;)

  • Existing users who need raster output from the CLI will be able to pipe to a utility like imagemagick to convert SVG to raster. If you were previously using

    badge build passed :green .gif

    this could be replaced by

    badge build passed :green | magick svg:- gif:-
  • No changes to shields.io: raster output will still be supported

@chris48s chris48s closed this as completed Apr 6, 2020
@paulmelnikow
Copy link
Member

I believe removal from the code base is still pending #4756.

Yea, that's true, though since the decision has been made I think we can close out the discussion. It seems like it'll be merged any day now 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm-package Badge generation and badge templates user-poll Voice your opinion!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants