-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[VisualStudioMarketplace] Add support to prerelease extensions version (Issue #8207) #8561
Conversation
…d pre-release example
… comment to explain the flags property calculations
…to match new logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this and including tests. It is looking in pretty good shape to start with and its clear you've worked through the documentation and contributing guidelines. 👍
const { extension } = this.transformExtension({ json }) | ||
const version = extension.versions[0].version | ||
const preRelease = 'Microsoft.VisualStudio.Code.PreRelease' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the intent here would be a bit clearer if we called this var preReleaseKey
or preReleaseTag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Fixed.
const version = includePrereleases | ||
? extension.versions[0].version | ||
: extension.versions.find( | ||
obj => !obj.properties.find(({ key }) => key === preRelease) | ||
).version | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a couple of problems here.
- If there are only pre-releases and no stable releases this will fail because
.find()
will just returnundefined
. For this case, we should fall back to showing the latest pre-release if there are no stable releases at all (this is what we normally do for other badges). - This one is a bit more of an edge case: It looks like the deal is pre-releases are indicated by
{"key": "Microsoft.VisualStudio.Code.PreRelease", "value": "true"}
and stable releases are indicated by complete absence of that object, but I think if we were to encounter a release with{"key": "Microsoft.VisualStudio.Code.PreRelease", "value": "false"}
we should also consider that stable (whether or not we currently think this is likely). This code would classify that as a pre-release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your comments.
I've changed the logic a bit so there will not be duplicated code:
- I've created a new logic which implements a fallback option.
- I now verify the value and not just the presence of the key.
Minor point: I've edited the PR title so it will run the tests for the whole VSCode Marketplace service family on the next push. This PR modifies the |
…elease key value is 'true'
…prerelease key has false value
@chris48s I've updated my code accordingly, and I've added 2 more tests -
I've also run the entire VisualStudioMarketplace tests locally without issues. Please let me know if there's anything else :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great first contribution 👍
Added pre-release version support to VScode marketplace service.
Updated response schema.
Updated Tests to include pre-release version.
PR fixing issue #8207