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

[FIX] ui5Framework: Allow providing exact prerelease versions #326

Merged
merged 4 commits into from
May 29, 2020

Conversation

matz3
Copy link
Member

@matz3 matz3 commented May 18, 2020

No description provided.

@coveralls
Copy link

coveralls commented May 18, 2020

Coverage Status

Coverage increased (+0.001%) to 89.848% when pulling bb8e1ef on fix-ui5framework-prerelease-resolution into 3017598 on master.

Copy link
Member

@RandomByte RandomByte left a comment

Choose a reason for hiding this comment

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

"latest" should not resolve with a prerelease version.

However, we should change the "versionRegExp" to allow for specifying prereleases. Same in ui5-cli:
https://github.com/SAP/ui5-cli/blob/90b1557faf72786bd87f50e121c8af769d14caca/lib/cli/commands/use.js#L22

@matz3
Copy link
Member Author

matz3 commented May 28, 2020

@RandomByte so I would add a static method to the AbstractResolver to reuse the validation check in the CLI.

@RandomByte
Copy link
Member

Sounds good to me 👍

Prereleases can now be passed, but "latest" won't resolve to them.
@matz3 matz3 force-pushed the fix-ui5framework-prerelease-resolution branch from 7bf2b61 to f23a2ca Compare May 28, 2020 11:51
@matz3 matz3 changed the title [FIX] ui5Framework: Fix version resolution of prereleases [FIX] ui5Framework: Allow providing exact prerelease versions May 28, 2020
@matz3
Copy link
Member Author

matz3 commented May 28, 2020

After looking into the code we don't need a version check in the CLI. See SAP/ui5-cli#341

@matz3 matz3 requested a review from RandomByte May 28, 2020 13:38
@@ -2,7 +2,13 @@ const path = require("path");
const log = require("@ui5/logger").getLogger("ui5Framework:AbstractResolver");
const semver = require("semver");

const versionRegExp = /^(0|[1-9]\d*)\.(0|[1-9]\d*)(?:\.(0|[1-9]\d*))?$/;
const VERSION_RANGE_REGEXP = new RegExp(
Copy link
Member

Choose a reason for hiding this comment

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

Where are these regex from? Or are they selfmade? If not, I would find a comment with their origin useful here

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I've added comments a test to check against the pattern from the JSON schema.

We might even just take the pattern from the JSON here instead of having a copy. What do you think?


// Reduced Semantic Versioning pattern
// Matches MAJOR.MINOR as a simple version range to be resolved to the latest patch
const VERSION_RANGE_REGEXP = /^(0|[1-9]\d*)\.(0|[1-9]\d*)$/;
Copy link
Member Author

@matz3 matz3 May 29, 2020

Choose a reason for hiding this comment

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

I was able to simplify this one as the full pattern also matches MAJOR.MINOR.PATCH

@matz3 matz3 merged commit 6ce985c into master May 29, 2020
@matz3 matz3 deleted the fix-ui5framework-prerelease-resolution branch May 29, 2020 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants