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

[greasyfork] Add Greasy Fork service badges #8080

Merged
merged 17 commits into from
Jun 16, 2022

Conversation

DenverCoder1
Copy link
Contributor

@DenverCoder1 DenverCoder1 commented Jun 13, 2022

Closes #8074
Partially resolved issues: #5505, #829

image

Reverted rating badges

image

@shields-ci
Copy link

shields-ci commented Jun 13, 2022

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

Generated by 🚫 dangerJS against 6f06d33

@calebcartwright calebcartwright added service-badge New or updated service badge core Server, BaseService, GitHub auth, Shared helpers and removed core Server, BaseService, GitHub auth, Shared helpers labels Jun 13, 2022
@calebcartwright
Copy link
Member

#5157 too

@calebcartwright
Copy link
Member

Could you help me understand the difference between daily and total installs, and why the total installs field is represented as users?

@DenverCoder1
Copy link
Contributor Author

DenverCoder1 commented Jun 13, 2022

Total installs = total number of installs
Daily installs = average installs per day

I suppose total installs could be under "downloads" instead of users.

I think I will change it to dt (consistent with Eclipse Marketplace)

I was basing it mostly on the "Mozilla Add-on" badges code.

@calebcartwright
Copy link
Member

I think I will change it to dt (consistent with Eclipse Marketplace)

Yeah let's do that, dd and dt variants would be consistent with how we handle downloads available over different intervals https://github.com/badges/shields/blob/master/doc/badge-urls.md#badge-url-conventions

@DenverCoder1
Copy link
Contributor Author

I have updated the code with the discussed change

Copy link
Member

@calebcartwright calebcartwright 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 this! This is looking good overall, with a few inline questions and requests for changes below

services/greasyfork/greasyfork-downloads.service.js Outdated Show resolved Hide resolved
services/greasyfork/greasyfork-downloads.service.js Outdated Show resolved Hide resolved
services/greasyfork/greasyfork-downloads.tester.js Outdated Show resolved Hide resolved
services/greasyfork/greasyfork-license.service.js Outdated Show resolved Hide resolved
services/greasyfork/greasyfork-license.service.js Outdated Show resolved Hide resolved
services/greasyfork/greasyfork-license.tester.js Outdated Show resolved Hide resolved
services/greasyfork/greasyfork-rating.service.js Outdated Show resolved Hide resolved
@calebcartwright
Copy link
Member

Everything else looks good to me, so happy to move forward once we get the ratings badges pulled out to a separate PR or issue for discussion

@DenverCoder1
Copy link
Contributor Author

Thanks, I have removed them from this PR

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

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

Thanks a ton! This has been requested a few times over the years so will be nice to get this to users

@Martii
Copy link

Martii commented Jun 14, 2022

@DenverCoder1

As I'm not on GF usually you may want to double check multiple licenses and any other key that normally supports more than a single JSON key value pair just like was done on OUJS. As I mentioned on the OUJS PR all key value pairs in all .user.js engines are required to be LIFO. I don't know if GF reverses the arrays for multiple key or not like they should be.

OUJS Admin

@DenverCoder1
Copy link
Contributor Author

DenverCoder1 commented Jun 14, 2022

@Martii

As I'm not on GF usually you may want to double check multiple licenses and any other key that normally supports more than a single JSON key value pair just like was done on OUJS. As I mentioned on the OUJS PR all key value pairs in all .user.js engines are required to be LIFO. I don't know if GF reverses the arrays for multiple key or not like they should be.

I have tested it and found that GF does not seem to do anything about multiple licenses. If the key appears multiple times, only the first license appears in the JSON.

As the value appears to only ever be a string, the solution I've done here seems to be the only option unless they change it on their end.

(eg: see https://greasyfork.org/en/scripts/446487/code?version=1060911 and https://greasyfork.org/en/scripts/446487.json?version=1060911)

@Martii
Copy link

Martii commented Jun 14, 2022

As the value appears to only ever be a string, the solution I've done here seems to be the only option unless they change it on their end.

Agreed. When he adopted our SPDX code implementation I think he may have missed that long time .user.js standard... perhaps even before that adoption.

@calebcartwright
Copy link
Member

TBH I'm not 100% sure I followed all the recent commentary, but it sounds like it's sorted?

@DenverCoder1 - I spotted one last minor thing, and if you could also rebase/merge to get your branch updated with the latest, or allow the maintainers to edit your PR (so our bots can do their thing) that'd be great. We have the branch protection policy enabled that requires branches be up to date, so that will be required for merging

Co-authored-by: Caleb Cartwright <[email protected]>
@DenverCoder1
Copy link
Contributor Author

Yeah, everything should be good.

I'm not sure I see an option to allow maintainer edits and I'm not sure why it would be disabled, but I've gone ahead and merged master.

@calebcartwright
Copy link
Member

Yeah, everything should be good.

I'm not sure I see an option to allow maintainer edits and I'm not sure why it would be disabled, but I've gone ahead and merged master.

This article may help, although there could just be something funky going on with GitHub

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

@DenverCoder1
Copy link
Contributor Author

This article may help, although there could just be something funky going on with GitHub

docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Yeah, I've looked before and I'm still not seeing that option. Possibly it could have to do with contributing from an organization account? I'm not sure.

@repo-ranger repo-ranger bot merged commit 8f7164f into badges:master Jun 16, 2022
@DenverCoder1 DenverCoder1 deleted the greasyfork branch June 16, 2022 01:19
@calebcartwright
Copy link
Member

Possibly it could have to do with contributing from an organization account? I'm not sure.

Yup that's it. Didn't realize your account was setup as an org. Will just be something to keep an eye on for current open PRs and any potential future ones

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Badge Request] Greasy Fork
4 participants