Use correct latest crates.io version

[nyurik]

For download count and license, get the correct latest version from the versions array.

Fix #9453

[github-actions]

	Warnings
⚠️	This PR modified service code for crates but not its test code. That's okay so long as it's refactoring existing code.

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

Generated by 🚫 dangerJS against 683a2c9

[calebcartwright]

I think it would probably be best to first take the simplest route with the existing problem on the yanked version (first non-yanked version), and then have a follow up discussion/potential PR for any logic changes.

[calebcartwright]

See also #8666, as it may be worth overhauling the selection logic and encapsulating it somewhere for usage across the relevant classes

[nyurik]

@calebcartwright thx for the link, it does confirm the same type of logic I used as used by the crates.io itself to compute latest stable vs latest, etc.

I do not think filtering out yanked versions is the right approach. I think everyone's expectation would be that shields show the same version as the default page on crates.io for all non-version-specific requests. The version shield already follows that logic, but the download count and license ones do not. Unfortunately I am not as familiar with ts as I would like, so I might not be the best person for overall code restructuring.

Here's the current logic of crates.io:

  /**
   * This is the default version that will be shown when visiting the crate
   * details page. Note that this can be `undefined` if all versions of the crate
   * have been yanked.
   * @return {string}
   */
  get defaultVersion() {
    if (this.max_stable_version) {
      return this.max_stable_version;
    }
    if (this.max_version && this.max_version !== '0.0.0') {
      return this.max_version;
    }
  }

I think shields should implement the same function in the base class, plus there should be another function defaultVersionObject which finds the .versions array entry that corresponds to the one returned by above defaultVersion().

Then all 3 classes would call it and act accordingly.

[nyurik]

oops, i think there was a in-flight issue - i tried to address the comments

[nyurik]

After a few back and forth, I think i got the more consolidated version - where the base class handles both the defaultVersion() -> string and defaultVersionObject() -> obj, and the 3 derived classes just use those functions directly. Seems far cleaner, and more consistent with the expectations

[nyurik]

@calebcartwright hi, are there any blockers for this PR? Anything I can improve?

[nyurik]

CC: @chris48s - not sure if that's the right person though, my apologies if not

[chris48s]

I don't mind jumping in on this one if I need to, but given @calebcartwright

• has made all the comment on this PR and Incorrect version shown for crates.io #9453 so far
• is actively involved in the rust community

I'm assuming he will do the final review on this one.

[calebcartwright]

Thanks @nyurik. We're a rather smaller maintainer team, so it's basically an "any maintainer, as and when they have time" review strategy vs. any specific person.

I haven't had a chance to go through the updated code yet, but will also note in response to expectations around versions is that one thing we have to balance is that Shields also has its own behavior around our version badges and we strive for consistency in our badges/badge behavior across the various services and ecosystem.

To be clear, I'm not pushing back on anything nor prescribing anything specific at this point, just noting that we have other factors and constraints our badge defaults work within separate from the target service we fetch data from.

[nyurik]

@calebcartwright thx for the response. I am not too certain I understood it though - shields already uses crate.io-provided "current version" as the default for one of the shields (get current version). What changes are you saying this PR should make in order to be merged? How would this be different from other shields? Thx!

[nyurik]

@calebcartwright hi, a friendly ping - is there anything i can help with this stalled PR? Please take a look at my last comment about behavior consistency, and how I think Rust shields can benefit from it. Thx!

[calebcartwright]

@calebcartwright thx for the response. I am not too certain I understood it though - shields already uses crate.io-provided "current version" as the default for one of the shields (get current version). What changes are you saying this PR should make in order to be merged? How would this be different from other shields? Thx!

I was simply sharing some additional context about this project, and that a large part of why it exists in the first place is to provide consistency in badges independent of, and occasionally intentionally in contradiction of, what the upstream systems of records do.

I.e. service/platform Foo doing X does not necessarily mean that we should/will do X too in our badge; we have to balance consistency across an additional layer

[calebcartwright]

As far as the PR itself, yes the review has stalled out but that's not on you and there's not really anything you can do to help.

We're essentially down to only 1 maintainer that's putting in time on the project consistently, and that's certainly had a detrimental effect on review capacity.

Looking back through the code after some significant amount of time, I can't help but feel like our entire codebase for providing download and version badges from crates.io is surprisingly, and hopefully unnecessarily, complex, and that the changes proposed here are exacerbating that complexity. E.g. the conditionality around the schema, the resultant necessitaty to perform falsy checks (including in some cases duplicatively)

My inclination at this point is actually to investigate a holistic change, and likely simplification, of our crates code, as I fear we're continuing to pile on to some code that's been around for a while and perhaps accounting for things that it doesn't necessarily need to anymore.

I recognize this suggested pivot would perpetuate the current state of an issue like #9453 where the incorrect stat is shown in cases where a crate has a "later" version with a different value and which has been yanked. However, I'm not convinced that's necessarily a common scenario, and I suspect there are some viable workarounds (e.g. /crates/dv/:crate/:version with a specific, correct version, or /crates/dr/:crate/, and/or GitHub or GitLab license badge for the associated repo)