search

badges / shields

Concise, consistent, and legible badges in SVG and raster format
https://shields.io
Creative Commons Zero v1.0 Universal
23.88k stars 5.51k forks source link

🎃 Convert Service Classes to Static Fields 🎃 #5555

Closed calebcartwright closed 4 years ago

calebcartwright commented 4 years ago

We recently completed the upgrade to Node 12 in our production environment which, among other benefits, now allows us to leverage static fields for defining core attributes like the category and route.

We've completed the refactoring of several service classes to use static fields, but there's a lot of them remaining and we could use your help to convert the rest!

What are the steps required?

This work involves updating the members in a service class (found in the *-service.js files) to convert the static getters to static fields,

For example:

  static get category() {
    return 'version'
  }

becomes

  static category = 'version'

If a service class doesn't have any getters (e.g. redirector classes like this one, then there's nothing needed for that class!

Additional information and references can be found in the corresponding section of the tutorial documentation as well previous PRs that did this conversion like #5552 or #5530

How to contribute?

Below is the list of remaining service class families (as a link to their location in source) that need to be refactored. If you're interested in working on this, leave a comment on this thread or drop us a note in Discord letting us know which one(s) you'd like to work on. Items that have been checked off have already been completed, but the others are up for grabs!

As a rule of thumb, we'd generally prefer to see PRs contain between 2-4 refactored service class families to keep the review and merge process streamlined. You can work on as many as you'd like, but please try to follow that guidance when contributing and split PRs accordingly!

jabbar86 commented 4 years ago

@calebcartwright I would like to contribute to this. Should I wait for hacktoberfest to start?

ThakurKarthik commented 4 years ago

Hi @calebcartwright I would love to work on some classes.I have posted the same message in discord with username cosmos

calebcartwright commented 4 years ago

@jabbar86

I would like to contribute to this.

Great! Please specify which of the service classes families you're going to work on so that we can prevent any duplicative efforts

Should I wait for hacktoberfest to start?

that's up to you! We'll happily accept help with this effort regardless of what month it is, but PRs submitted before Hacktoberfest begins will obviously not count towards your Hacktoberfest contributions

@ThakurKarthik

I would love to work on some classes.I have posted the same message in discord with username cosmos

Great! I've updated the above list to reflect the ones you've claimed

jabbar86 commented 4 years ago

@calebcartwright Thanks I will contribute before hacktoberfest too. I will be working on.

homebrew
hsts
itunes
jenkins
jetbrains
ThakurKarthik commented 4 years ago

@calebcartwright can i work on this jira jitpack jsdelivr keybase , if no one else is working on those?

jabbar86 commented 4 years ago

@calebcartwright I have finish https://github.com/badges/shields/issues/5555#issuecomment-695256771. I would like to take another modules i.e 

lgtm
liberapay
librariesio
localizely
luarocks
jabbar86 commented 4 years ago

@calebcartwright I have completed https://github.com/badges/shields/issues/5555#issuecomment-695775615. Assign me 

maven-metadata
microbadger
mozilla-observatory
myget
netlify

If anyone hasn't started working on it.Thanks

jabbar86 commented 4 years ago

@calebcartwright Assign Me 


nexus
node
nodeping
npm
nsp
ThakurKarthik commented 4 years ago

I am working on these services [nuget nycrc opencollective opm]

monizb commented 4 years ago

@calebcartwright I would like to take up:

website
wercker
youtube
wheelmap
wordpress

Thanks :)

vanhaaggen commented 4 years ago

@calebcartwright can i help out with these?

snyk | sonar | sourceforge | sourcegraph | spack

ThakurKarthik commented 4 years ago

Hey @calebcartwright if no one taken [ osslifecycle packagecontrol packagist pkgreview poeditor ] already I would like to work on them. Thanks !

ThakurKarthik commented 4 years ago

@RDIL feel free to pick any 4 - 5 services which are not done and post them here or at shields discord channel to prevent duplicate work.Thanks !

ThakurKarthik commented 4 years ago

@calebcartwright I am taking these files next [ powershellgallery pub puppetforge pypi ].

ThakurKarthik commented 4 years ago

@calebcartwright I have changes for the above services also.Meanwhile I am working on these services [readthedocs reddit redmine repology] and open PR once the previous changes get's accepted. Thanks !

ThakurKarthik commented 4 years ago

I am working on these too [ requires resharper,scoop scrutinizer] Thanks !

EDIT: @calebcartwright looks like there is nothing to work in resharper and scoop services. Please mark them as completed.

I am working on [security-headers, shippable] services

@calebcartwright please add these [security-headers, shippable] services in the WIP list.I have these changes already ready with me and waiting for my other pull request to be merged

jabbar86 commented 4 years ago

@calebcartwright I will be working in 2 parts so that its easy to maintain the code. Let me know if someone has taken any below service so that I can remove it and take another one.

1st part 

swagger
symfony
teamcity
travis
treeware

2nd part 


twitch
twitter
ubuntu
uptimerobot
vaadin-directory

Thanks.

BalgopalPatro commented 4 years ago

@calebcartwright I wanna do for the followings visual-studio-app-center visual-studio-marketplace w3c

MonliH commented 4 years ago

I would like to help as well, although it seems all the badges have been "claimed".

calebcartwright commented 4 years ago

@MonliH - so it seems! If you're interested there's tons of other Shields-related work in need of assistance

https://github.com/badges/shields/issues?q=is%3Aissue+is%3Aopen+label%3A%22good+first+issue%22

ThakurKarthik commented 4 years ago

Hi @calebcartwright quick update here this PR https://github.com/badges/shields/pull/5635 already fixed the [powershellgallery, pub, puppetforge, pypi] services. You can mark them done. Thanks !

BalgopalPatro commented 4 years ago

@calebcartwright I wanna do for the followings visual-studio-app-center visual-studio-marketplace w3c

@calebcartwright Please review completed task with PR #5642

PyvesB commented 4 years ago

Things are getting along quite nicely, thanks to all those involved so far! 🙌🏻

JoeIzzard commented 4 years ago

@monizb I didn't see this thread until after I had submitted the PR's. Wordpress has been done, partly as I was adding badges and partly as it's own PR. Related PR's:

Sorry for the inconvenience! Hopefully it's not wasted any work you've done.

jabbar86 commented 4 years ago

@JoeIzzard if it's still not completed I can take up this task.

JoeIzzard commented 4 years ago

@jabbar86 Those PR's should resolve the WordPress service completely.

geekyrajshri commented 4 years ago

@calebcartwright I would like to contribute on any tasks left on this. Let me know if that's available

calebcartwright commented 4 years ago

Thanks @geekyrajshri! At this point all the service classes enumerated above have either been resolved or are already pending by another user. However, I do believe there's a handful of service classes in the GitHub family that I thought I would have already finished when I made the original list in the issue description, but I didn't end up finishing theme all.

That means that most/all of the service classes inclusively from GitHubSearch (https://github.com/badges/shields/blob/master/services/github/github-search.service.js) through GitHubWorkFlowStatus probably still need to be refactored if you (or anyone else) wants to work on those

vividh commented 4 years ago

Wondering why a codemod wasn't used for this?

calebcartwright commented 4 years ago

Wondering why a codemod wasn't used for this?

@vividh - what exactly is the motivation for the question, are you just curious about why this work was completed the way it was? The short answer is that it made more sense for us to solicit assistance from our community of contributors vs a single maintainer figuring out how to automate everything.

We review each and every change in detail to maintain the stability, reliability, and quality of the Shields.io service. And while the nature of these changes is rather trivial, we also knew it was never going to be feasible to update and review all of these files in one pass because those trivial changes still produce large diffs.

As such we also knew this work was going to have to be done over many PRs and not something we wanted to stick with a single person, hence the call for assistance from the community.

From the perspective of the Shields maintainers, it doesn't really matter to us how folks helping us with this issue made the changes (whether manually by hand or automation), but it definitely wasn't going to be an effective use of our time to research, select, document, and recommend a codemod/AST-based automation approach for individual contributors to attempt to adopt.. Especially since most of these contributors would likely have found that to be more time consuming and challenging than tweaking 4 or 5 files by hand anyway.

vividh commented 4 years ago

Thanks for the clarification @calebcartwright. My question was borne out of curiosity, since I didn't see any discussions of codemods on this issue, and wanted to understand if it was considered.

The way you put it though makes sense to me. Codemods don't have the best documentation, and recommending to use that would possibly have alienated many potential contributors.

calebcartwright commented 4 years ago

@monizb - are you still working on the last set of service class files as you previously indicated? We'd love to get this wrapped up before October ends if possible!

vividh commented 4 years ago

I can take these up if @monizb is unable to.

homersimpsons commented 4 years ago

@calebcartwright This issue might be done as #5793 is merged.

I did mention in the Pull Request

Searching for static get still gives some results that could be migrated to static variable = ...

Should I address the remaining statics ?

calebcartwright commented 4 years ago

@calebcartwright This issue might be done as #5793 is merged.

Thanks for the follow up @homersimpsons!

Should I address the remaining statics ?

We wanted to address this refactoring in buckets, with the first being core (which is done) and then the service classes. There's a few static getters on the master branch in the package (under badge-maker) but do not bother with any of those, as there's a separate sizeable refactoring of that code inflight that has already done so (refs #5754).

There does appear to be a handful of service classes that were missed altogether for one reason or another, like aur and jenkins-coverage, as well as a few fields that were missed in classes that were refactored. If you'd like to work on any of those returning a static value then that'd be great! Be sure to split changes up into multiple PRs once the diffs start to get over a few hundred lines to make them easier to review.

Please also note though that Discord was intentionally skipped for now to minimize merge conflicts with #5079, and that not every single static get instance can/should be converted, for example:

https://github.com/badges/shields/blob/5e335e1bb2601f3cc27aee9c67e5a7fe9e309a3c/services/youtube/youtube-comments.service.js#L11-L25

paulmelnikow commented 4 years ago

There's a few static getters on the master branch in the package (under badge-maker) but do not bother with any of those

Just a heads up that they also have to be left for now because we still support Node 10 for the package.

calebcartwright commented 4 years ago

Thanks to everyone that helped with this work! Seems all the service class files have been refactored now, so going to go ahead and close.