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

[GradlePluginPortal] add gradle plugin portal #6449

Merged
merged 12 commits into from
Jun 7, 2021

Conversation

anatawa12
Copy link
Contributor

Closes #1125

@shields-ci
Copy link

shields-ci commented May 5, 2021

Messages
📖 ✨ Thanks for your contribution to Shields, @anatawa12!

Generated by 🚫 dangerJS against 74ede93

@anatawa12 anatawa12 changed the title [gradle-plugin-portal] add gradle plugin portal [GradlePluginPortal] add gradle plugin portal May 5, 2021
@PyvesB PyvesB added the service-badge Accepted and actionable changes, features, and bugs label May 6, 2021
Copy link
Member

@PyvesB PyvesB left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution to Shields.io @anatawa12! I'm kind of in two minds here.

On the one hand it does make building badges for Gradle Plugin Portals slightly simpler, as you don't need to specify the full Maven metadata URL. The resulting badge URL is consequently shorter.

However, it does so at the cost of additional maintenance on our side. In particular, this new implementation is pretty much a copy-paste of https://github.com/badges/shields/tree/master/services/maven-central, with only a few keywords changed. I would personally not be keen on merging this pull request in its current state.

I'm wondering whether adding a specific Gradle Plugin Portal example to the existing Maven Central implementation would be enough here. This example would effectively act as documenting the solution described in #1125 (comment) directly in our homepage.

Thoughts from @badges/shields-maintainers also welcome!

@chris48s
Copy link
Member

chris48s commented May 6, 2021

I don't think I understand the nuances of the java ecosystem well enough to have much of an opinion on this so I'll defer to your judgement @PyvesB but a couple of things that occur to me:

  • If this is essentially a subset of maven (I'm assuming this given you say we could also solve this with documentation), could we implement this as a redirect route to the maven badge?
  • If we want to include grade plugin as a different implementation, we could at least pull out the schema and the logic in handle() into shared helpers which both gradle and maven share which would make the implementation pretty thin

@PyvesB
Copy link
Member

PyvesB commented May 6, 2021

could we implement this as a redirect route to the maven badge?

This would probably be a neat and concise solution, but do you know how we would go about surfacing an example in the UI? Our redirector functionality is generally used for hiding a legacy route, rather than advertising a new one.

@anatawa12
Copy link
Contributor Author

If we want to include grade plugin as a different implementation, we could at least pull out the schema and the logic in handle() into shared helpers which both gradle and maven share which would make the implementation pretty thin

In my opinion, This solution is better because maven-central and gradle plugin portal are same except for url syntax and label. It may also good to put logic for maven-metadata into shared helpers. Both maven-metadata and maven-central have same schema for xml. I don't know why not currently but maven-metadata should also have version prefix so It's good to mix into single logic.

@anatawa12
Copy link
Contributor Author

In my opinion, having gradle logo by default is good. However, by the CONTRIBUTING.md, It looks not allowed. What do you think to have gradle logo by default? Is it allowed to have gradle logo in example?

@shields-cd shields-cd temporarily deployed to shields-staging-pr-6449 May 8, 2021 02:55 Inactive
@calebcartwright
Copy link
Member

I'm wondering whether adding a specific Gradle Plugin Portal example to the existing Maven Central implementation would be enough here

I think this would be helpful in the event we're unable to incorporate any other changes. Adding this as an example, with relevant search terms, and any docs for the badge builder window should help folks discover and utilize the badge.

However, it does so at the cost of additional maintenance on our side. In particular, this new implementation is pretty much a copy-paste of https://github.com/badges/shields/tree/master/services/maven-central, with only a few keywords changed. I would personally not be keen on merging this pull request in its current state.

I agree. I'm not necessarily opposed to having a "native" badge for gradle plugins and a corresponding route, but we'd need an implementation that encapsulated all the common bits (and there indeed seems to be a ton of overlap). The first draft proposed here would have too much code duplication for my liking.

If we want to include grade plugin as a different implementation, we could at least pull out the schema and the logic in handle() into shared helpers which both gradle and maven share which would make the implementation pretty thin

Agreed 💯

Our redirector functionality is generally used for hiding a legacy route, rather than advertising a new one.

Yeah that sounds like it'd be tricky

@calebcartwright
Copy link
Member

In my opinion, having gradle logo by default is good. However, by the CONTRIBUTING.md, It looks not allowed. What do you think to have gradle logo by default? Is it allowed to have gradle logo in example?

That's correct @anatawa12, we don't do logos by default and we cannot do so here either. Folks that want to include a logo can do so easily, but this badge would be subject to the same set of rules that govern the other badges.

@chris48s
Copy link
Member

chris48s commented May 8, 2021

do you know how we would go about surfacing an example in the UI?

This is a good point. I don't think we can accommodate that with the current setup but if we wanted I think it would be possible to allow redirector to accept examples as a param and then set it on the service we return

module.exports = function redirector(attrs) {
const {
name,
category,
route,
transformPath,
transformQueryParams,
overrideTransformedQueryParams,
} = Joi.attempt(attrs, attrSchema, `Redirector for ${attrs.route.base}`)
return class Redirector extends BaseService {
static name =
name ||
`${camelcase(route.base.replace(/\//g, '_'), {
pascalCase: true,
})}Redirect`
static category = category
static isDeprecated = true
static route = route

@PyvesB
Copy link
Member

PyvesB commented May 25, 2021

Any thoughts on the above @anatawa12? Would you be keen on trying the redirect route approach with the pointers that Chris has provided?

@anatawa12
Copy link
Contributor Author

@PyvesB I'm sorry long time no response. I made gradle-plugin-portal as a redirector. Could you check my code? and Should I rewite maven-central with redirector?

@PyvesB
Copy link
Member

PyvesB commented Jun 2, 2021

Quite a lot of my plate at the moment, but I'll try to review this as soon as possible. If you want to refactor maven-central as well, I would suggest doing that in a follow up pull request. 😉

@shields-cd shields-cd temporarily deployed to shields-staging-pr-6449 June 5, 2021 10:42 Inactive
},
/*
{
title: 'Gradle Plugin Portal with version prefix filter',
Copy link
Member

@PyvesB PyvesB Jun 5, 2021

Choose a reason for hiding this comment

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

Please remove this commented example for now. When and if we get round to adding a version prefix attribute to maven-metadata, it's easy enough to re-add at that point, without the risk of the commented code having gone stale in the meantime. 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this comment.

@shields-cd shields-cd temporarily deployed to shields-staging-pr-6449 June 7, 2021 05:23 Inactive
Copy link
Member

@PyvesB PyvesB left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me! 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add badge for gradle plugin portal
6 participants