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.

Sign up for GitHub

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

Jump to bottom

[Scoop] Added scoop-license badge. #10627

Merged
merged 9 commits into from
Oct 25, 2024
Merged

[Scoop] Added scoop-license badge. #10627

merged 9 commits into from
Oct 25, 2024
+247 −73

Conversation

MohanKumarAmbati
Copy link
Contributor

Hi Team,

Added scoop-license badge.

closes - #10297

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 20, 2024 •
edited
Loading

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

Generated by 🚫 dangerJS against ed5bad0

@chris48s chris48s added the service-badge New or updated service badge label Oct 21, 2024
@chris48s
Copy link
Member

There is a lot of code here which is straight copy/pasted from the scoop version service.

I think if we're going to merge this I would like to see some of the common code extracted into a shared helper with a ScoopBase class which both ScoopLicense and ScoopVersion inherit from.

Specifically, several of the schemas and bucket file cache can be shared and nearly all of the logic in handle() can be extracted to a shared async fetch() method on ScoopBase.

@MohanKumarAmbati
Copy link
Contributor Author

Totally agree with you Chris. I had this thought in mind, but I'm afraid it will break the existing ones let me give it a try though 🙂👍.

@chris48s
Copy link
Member

Running the existing service tests for scoop version should help you ensure we're not breaking the existing code as you refactor. You should also be able to look at some other services where we have a base class and multiple services extending it to see some examples. If you get stuck, feel free to ask for help.

@MohanKumarAmbati
Copy link
Contributor Author

Chris, refactored into scoop-base according to the suggestions.

jNullj
jNullj suggested changes Oct 23, 2024
View reviewed changes
Copy link
Collaborator

@jNullj jNullj left a comment •
edited
Loading

Choose a reason for hiding this comment

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

removed

jNullj
jNullj reviewed Oct 23, 2024
View reviewed changes
services/scoop/scoop-license.service.js Outdated
Comment on lines 23 to 26
// The buckets file (https://github.com/lukesampson/scoop/blob/master/buckets.json) changes very rarely.
// Cache it for the lifetime of the current Node.js process.
buckets = null

Copy link
Collaborator

Choose a reason for hiding this comment

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

i think this should move to base and was left out in the refactor

@MohanKumarAmbati
Copy link
Contributor Author

removed

Didn't get you, Jonathan.

@jNullj
Copy link
Collaborator

jNullj commented Oct 23, 2024

removed

Didn't get you, Jonathan.

I removed that comment, it was made by mistake and you can ignore it. Sorry for not writing that more clearly.
Wrote that as there was no option to delete it.
Please see the suggestions added after my review.

@MohanKumarAmbati
Copy link
Contributor Author

removed

Didn't get you, Jonathan.

I removed that comment, it was made by mistake and you can ignore it. Sorry for not writing that more clearly.
Wrote that as there was no option to delete it.
Please see the suggestions added after my review.

Made changes - refactoring of description. Am I missing something else, please let me know if any.

jNullj
jNullj suggested changes Oct 23, 2024
View reviewed changes
Copy link
Collaborator

@jNullj jNullj left a comment

Choose a reason for hiding this comment

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

Didn't notice the review was in draft, now you can see my review suggestions.

services/scoop/scoop-license.tester.js Outdated Show resolved Hide resolved
services/scoop/scoop-version.service.js Outdated Show resolved Hide resolved
services/scoop/scoop-base.js Show resolved Hide resolved
jNullj
jNullj suggested changes Oct 23, 2024
View reviewed changes
Copy link
Collaborator

@jNullj jNullj left a comment

Choose a reason for hiding this comment

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

Also this one

services/scoop/scoop-base.js Show resolved Hide resolved
@MohanKumarAmbati
Copy link
Contributor Author

Got it, will work on that.

@MohanKumarAmbati @jNullj
Update services/scoop/scoop-license.tester.js
9b68c04 
Co-authored-by: jNullj <15849761+jNullj@users.noreply.github.com>
@MohanKumarAmbati
Copy link
Contributor Author

@jNullj , changes are done accordingly. Can you please review it once and let me know if there are any additional changes required.

jNullj
jNullj approved these changes Oct 24, 2024
View reviewed changes
Copy link
Collaborator

@jNullj jNullj left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for taking the time and applying these changes.
image

@github-actions GitHub Actions
Copy link
Contributor

🚀 Updated review app: https://pr-10627-badges-shields.fly.dev

@chris48s
Copy link
Member

Thanks for picking up the review on this one @jNullj

I've just had a look over this. I have one further request for an improvement: Can we move queryParamSchema to scoop-base.js and import it in both the version and license files please.

After, its good to go.

Nice work, both.

@MohanKumarAmbati
Copy link
Contributor Author

I've just had a look over this. I have one further request for an improvement: Can we move queryParamSchema to scoop-base.js and import it in both the version and license files please.

  • Changes are made accordingly - ed5bad0.

chris48s
chris48s approved these changes Oct 25, 2024
View reviewed changes
Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

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

thanks

@chris48s chris48s added this pull request to the merge queue Oct 25, 2024
Merged via the queue into badges:master with commit 5e40080 Oct 25, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chris48s chris48s chris48s approved these changes

@jNullj jNullj Awaiting requested review from jNullj

Assignees
No one assigned
Labels
service-badge New or updated service badge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MohanKumarAmbati @chris48s @jNullj