ropensci-org / badges

Badges for packages in rOpenSci software peer-review
3 stars 0 forks source link

new badges for stats peer review #2

Closed mpadge closed 2 years ago

mpadge commented 3 years ago

@sckott We would now like to incorporate the new stats peer review badges here. There are 3 new badges:

stats-bronze stats-silver stats-gold

Your current code here simply associates the standard "Approved" label with the standard "Peer-reviewed" svg. We have not to date considered different versions of the "Approved" label itself; only that approval will be triggered by different commands of @ropensci-review-bot approve bronze/silver/gold. I guess the easiest thing would be to translate these into distinct issue labels, so they could then be directly used in the update_badges.rb script? That could all then simply extend the current "status" variable, so it could take these three new values as extensions of "approved". The JSON format would then not have to change. Does that sound workable?


A second problem we would then have would be the versioning. As standards develop, numbers of badges will increase, retaining one set of 3 badges for each preceding iteration of standards. We're still working out the best way to track and record this. The version data should be automatically generated in response to the final approve command, so the bot will declare the current version of standards for which the software is approved. I guess we could do that in a standard way so it could be grepped in update_badges to extract the associated version? There'll also be possibilities for authors to update their software to align with more recent standards, so the command to approve standards alignment will then be repeated and will generate a second version of the same statement.

One direct way to achieve this would be to also have versioned forms of the issue labels themselves, so there would be approved-stats-bronze-0.0.1 and so on. Bot commands could be used to directly update the issue labels, and so update_badges would then only need to find an approved label and then map the full name of the label on to the name of associated svgs. Only problem there would be proliferation of issue labels, so would definitely like your feedback there before thinking any further. Ping @noamross

noamross commented 3 years ago

Good summary, Mark. I don't see a problem with a greater proliferation of badges. Go with that at first pass.

That said, it would be good to think about an immutable registry that associates an approval with a hash of both the package commit/version and the standards commit/version.

sckott commented 3 years ago

The first part sounds good mark.

A few things to note in case you weren't aware:

maelle commented 3 years ago

That said, it would be good to think about an immutable registry that associates an approval with a hash of both the package commit/version and the standards commit/version.

@noamross is this something that should be added as a feature for the Airtable-related buffy stuff?

sckott commented 3 years ago

@mpadge can work on this now. Any response to my questions above?

Some more thoughts/questions:

mpadge commented 3 years ago

Thanks @sckott, some responses to your questions:

this repo uses the CNAME badges.ropensci.org. is that okay or do you want a different base url?

That one's great

perhaps its easiest to have a separate ruby script for stats software review, copying update_badges.rb, then run it in the action file after or before this line https://github.com/ropensci-org/badges/blob/main/.github/workflows/badges.yml#L26 - might be a bit cleaner to separate concerns a bit while keeping

I don't have a firm opinion either way here, although suspect it might actually be cleaner simply modifying current script, because it would then be more obvious how the two workflows are related. We'd then just need to extend this line:

https://github.com/ropensci-org/badges/blob/0047587bfa4d5ba8215c4589b01da7ae044db787/update_badges.rb#L18

to concatenate the new badges into iss_peer_rev. The stats_checks array that follows would then just need the new lines for grep(/approved-stats-bronze-0.0.1/) and so on. So modifying current script would require only two additional lines for each new badge type, whereas script duplications would be adding far more code. Does that sound about right?

this currently runs on a cron schedule https://github.com/ropensci-org/badges/blob/main/.github/workflows/badges.yml#L8 - do you need badges to be updated/changed in response to the bot? i imagine that can be done but is not set up to do that now

Daily updates should be fine.


As for workflow updates and re-enabling, i guess that would be an argument for at least having any @ropensci-review-bot approve commands enable the workflow, right? That should be easy for @xuanxu to incorporate

sckott commented 3 years ago

Okay, I'll start a new branch, modifying the current ruby script, and we can discuss in PR

Do any of the labels approved-stats-bronze etc. exist yet?

that would be an argument for at least having any @ropensci-review-bot approve commands enable the workflow

sounds like a good idea

mpadge commented 3 years ago

Do any of the labels approved-stats-bronze etc. exist yet?

Nope, not yet, because we need this discussion to work out whether the labels themselves need to be versioned, or whether we can get away with just having generic labels. The latter would be better, but we then somehow need a way for the package.json to be constructed with a record of current or last compliant version.

sckott commented 3 years ago

... with a record of current or last compliant version

How do I get that information?

sckott commented 3 years ago

@mpadge Okay, now I've edited code in the Ruby script to add a new field to the json file, so each entry looks like:

{
    "pkgname": "bizbaz",
    "submitter": "janedoe",
    "iss_no": 10,
    "status": "reviewed",
    "version": "0.1.14",
    "stats_version": "0.0.1"
}

Thoughts?

mpadge commented 3 years ago

That looks like it should all do the job. Sorry i somehow missed your question on standards version - the definitive value will be that on the badge, so this should all work perfectly as is. You only need to worry about the badge; and tweaks to approved version compliance will be handled by buffy, via commands that themselves will directly change the badge.

sckott commented 3 years ago

Okay, sounds good. Thanks.