use new plugin api for [jetbrains] #5974
Conversation
| static _cleanPluginId(pluginId) { | ||
| const match = pluginId.match(/^([0-9])+/) | ||
| if (match) { | ||
| return match[0] | ||
| } | ||
| return pluginId | ||
| } |
There was a problem hiding this comment.
Do we know definitively that for any 1234-foo-bar plugin value the leading numeric portion will always match the underlying plugin id?
I.e. could a valid long form be 1234-foo-bar but the corresponding plugin id be 4567?
Based on some admittedly anecdotal testing I've done that does seem to be true, but if there's any conclusive doc/material from JetBrains that'd be helpful (an inline comment may be beneficial one way or the other for posterity)
There was a problem hiding this comment.
There's nowhere it is documented explicitly, but given that these (for example) do the same thing:
https://plugins.jetbrains.com/plugins/list?pluginId=1347-scala
https://plugins.jetbrains.com/plugins/list?pluginId=1347foobar
I'm pretty confident that the string part is irrelevant.
There was a problem hiding this comment.
That's so strange on the JetBrains side (accepting any randomness after the id) 😆
| if (this.constructor._isLegacyPluginId(pluginId)) { | ||
| const intelliJPluginData = await this.fetchIntelliJPluginData({ | ||
| pluginId, | ||
| schema: intelliJschema, | ||
| }) | ||
| rating = | ||
| intelliJPluginData['plugin-repository'].category['idea-plugin'][0] | ||
| .rating | ||
| } else { | ||
| const jetbrainsPluginData = await this._requestJson({ | ||
| schema: jetbrainsSchema, | ||
| url: `https://plugins.jetbrains.com/api/plugins/${this.constructor._cleanPluginId( | ||
| pluginId | ||
| )}/rating`, | ||
| errorMessages: { 400: 'not found' }, | ||
| }) |
There was a problem hiding this comment.
Would it be worthwhile to pull any of this up into the base class or helper functions? Seems like there will be a lot of repetition for at least some parts (e.g. the json calls)
There was a problem hiding this comment.
I did consider this, but given that downloads/rating/version each query a different API endpoint and need a different schema, I don't think there is much scope to do that - the only thing you could move up a layer is errorMessages: { 400: 'not found' }. When the old API goes away I think the natural implementation is probably that we just bin JetbrainsBase and have the 3 badge classes extend BaseJsonService directly. I think this layout will make that change easier in future.
There was a problem hiding this comment.
Doesn't matter all that much to me, but seems like the only differences are the value for the schema and an optional/additional trailing path segment on the uri which could both be handled in a single foo/fetch type function (we do something similar for Jenkins with more complex variances on the calls)
This PR implements the solution I suggested in #5946 (comment) - use the new API if we've got a numeric ID. Fall back to the old (IntelliJ only) API otherwise.
This is a bit messy because the old API is XML-based whereas the new one uses JSON so I've made a base class that inherits from
BaseXmlServiceand given it some methods to call/parse/validate JSON. At some point in future when the XML API is deprecated and we drop the string ID format we can just switch everything to inherit fromBaseJsonServiceand tidy this up a bit.closes #5946