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

[factorio-mod-portal] services #8625

Merged
merged 17 commits into from
Nov 16, 2022

Conversation

kuanoni
Copy link
Contributor

@kuanoni kuanoni commented Nov 14, 2022

Saw this #8582 issue and thought I'd take a crack at closing it. I'm new to open-source contributions so I'm eager to hear any feedback/changes.

image

API: https://mods.factorio.com/api/mods/{name} where {name} is the name of the mod, e.g.
https://mods.factorio.com/api/mods/rso-mod

No authentication is required.
API is documented here: https://wiki.factorio.com/Mod_portal_API#/api/mods/

@kuanoni kuanoni changed the title Factorio mod portal services [FactorioModPortal] services Nov 14, 2022
@shields-ci
Copy link

shields-ci commented Nov 14, 2022

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

Generated by 🚫 dangerJS against 3bc682a

@chris48s chris48s changed the title [FactorioModPortal] services [factorio-mod-portal] services Nov 14, 2022
@chris48s chris48s added the service-badge Accepted and actionable changes, features, and bugs label Nov 14, 2022
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.

Good start. Thanks for reading the contributing docs and including tests. It is also great to see we are leaning on the common helper functions like renderVersionBadge() and renderDownloadsBadge() for consistency 👍

import { renderVersionBadge } from '../version.js'

const schema = Joi.object({
downloads_count: Joi.number().required(),
Copy link
Member

Choose a reason for hiding this comment

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

Lets validate this using nonNegativeInteger

}).required()

// Factorio Mod portal API
// @see https://wiki.factorio.com/Mod_portal_API
Copy link
Member

Choose a reason for hiding this comment

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

👍

// Badge for mod's latest compatible Factorio version
// Query 'range' to display range of compatible versions
class FactorioModPortalFactorioVersions extends BaseFactorioModPortalService {
static category = 'version'
Copy link
Member

Choose a reason for hiding this comment

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

Category for this one should be static category = 'platform-support'

static category = 'version'

static route = {
base: 'factorio-mod-portal/latest-version',
Copy link
Member

Choose a reason for hiding this comment

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

Normally we use /v as the route for version badges - see https://github.com/badges/shields/blob/master/doc/badge-urls.md

static category = 'downloads'

static route = {
base: 'factorio-mod-portal/downloads',
Copy link
Member

Choose a reason for hiding this comment

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

Normally we use /dt as the route for a badge showing total downloads - see https://github.com/badges/shields/blob/master/doc/badge-urls.md

@@ -0,0 +1,50 @@
import {
isComposerVersion,
Copy link
Member

Choose a reason for hiding this comment

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

isComposerVersion is a fairly specific validator for PHP package ranges. It looks like what you really want here is probably isVPlusDottedVersionNClauses or isVPlusDottedVersionAtLeastOne.


async handle({ modName }, queryParams) {
const { first_release, latest_release } = await this.fetch({ modName })
const range = queryParams.range !== undefined
Copy link
Member

Choose a reason for hiding this comment

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

Generally for platform & version support badges, we just show compatibility with the base platform for the latest release of the target package/plugin/whatever (in this case, the factorio version(s) that the latest release of the mod is compatible with). I think I'd be in favour of dropping the range param and sticking with that here.

return {
downloads_count,
first_release: releases[0],
latest_release: releases[releases.length - 1],
Copy link
Member

Choose a reason for hiding this comment

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

Do we have complete confidence this array is sorted by version? The docs aren't completely clear.

Copy link
Contributor Author

@kuanoni kuanoni Nov 14, 2022

Choose a reason for hiding this comment

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

I couldn't find in the docs or anywhere else that explicitly says it's sorted by version, but I made a small script to run through every mod listed and can confirm that the releases array is always sorted by version.

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.

Nice. This one's good to go. Great first contribution

@repo-ranger repo-ranger bot merged commit 755af71 into badges:master Nov 16, 2022
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.

3 participants