Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

added 'semver' as core type accelerator for S.M.A.SemanticVersion#4142

Merged
daxian-dbw merged 1 commit into
PowerShell:masterPowerShell/PowerShell:masterfrom
oising:semver-acceleratoroising/PowerShell:semver-acceleratorCopy head branch name to clipboard
Jul 11, 2017
Merged

added 'semver' as core type accelerator for S.M.A.SemanticVersion#4142
daxian-dbw merged 1 commit into
PowerShell:masterPowerShell/PowerShell:masterfrom
oising:semver-acceleratoroising/PowerShell:semver-acceleratorCopy head branch name to clipboard

Conversation

@oising

@oising oising commented Jun 29, 2017

Copy link
Copy Markdown
Contributor

As per #3460

Added [semver] as a core (safe to use in constrained language mode) type accelerator for System.Management.Automation.SemanticVersion.

Individual accelerators are not currently being tested, so no unit tests were added or updated.

@iSazonov

Copy link
Copy Markdown
Collaborator

Appveyor temporary failed.

@oising

oising commented Jun 29, 2017

Copy link
Copy Markdown
Contributor Author

Yeah, it seems one of the New-TimeSpan tests is failing. It must have already been broken when I merged to rebase.

@daxian-dbw daxian-dbw requested a review from PaulHigin June 29, 2017 17:38
@daxian-dbw

Copy link
Copy Markdown
Member

@PaulHigin currently SemanticVersion is permitted in constrained language, please review if that's OK.

@mirichmo mirichmo requested a review from lzybkr June 29, 2017 23:13
@PaulHigin

Copy link
Copy Markdown
Contributor

@oising PowerShell core types are intended to be the absolute minimum needed in a constrained language interactive session. Can you tell us the scenario where this type is needed and why constrained language is required?

@oising

oising commented Jun 30, 2017

Copy link
Copy Markdown
Contributor Author

@PaulHigin I guess I included it in core types for the same reasons you guys included [ModuleSpecification]. Given that NuGet packages are a core (no pun intended) building block of CoreFX, and [semver] being a large part of their identity, it seemed like something that may be more useful than not. I know this is all a bit fuzzy, but it's just what made sense to me.

@daxian-dbw

Copy link
Copy Markdown
Member

@PaulHigin I think the rationale is that SemanticVersion is replacing System.Version ( the type of $PSVersionTable.PSVersion is SemanticVersion), so I think it makes sense to allow it in constrained language. But we need your expertise to inspect the implementation of SemanticVersion and see if it's safe to be exposed in constrained language.

@oising

oising commented Jun 30, 2017

Copy link
Copy Markdown
Contributor Author

@daxian-dbw Yes, also good points. I took a cursory glance at the implementation and didn't see any red flags, but I'm also not privy to your criteria.

@PaulHigin

Copy link
Copy Markdown
Contributor

Ok, I think this is reasonable. I'll review for security safety. Thanks.

@daxian-dbw daxian-dbw self-assigned this Jul 5, 2017

@PaulHigin PaulHigin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@daxian-dbw

Copy link
Copy Markdown
Member

Filed #4221 to track adding tests for type accelerators in PowerShell.

@daxian-dbw daxian-dbw merged commit d9828fe into PowerShell:master Jul 11, 2017
@oising oising deleted the semver-accelerator branch August 21, 2018 17:50
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.

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.