8000 added 'semver' as core type accelerator for S.M.A.SemanticVersion by oising · Pull Request #4142 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 11, 2017
Merged

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

merged 1 commit into from
Jul 11, 2017

Conversation

oising
Copy link
Contributor
@oising oising commented Jun 29, 2017

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
Collaborator

Appveyor temporary failed.

@oising
Copy link
Contributor Author
oising commented Jun 29, 2017

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
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
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
Copy link
Contributor Author
oising commented Jun 30, 2017

@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
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
Copy link
Contributor Author
oising commented Jun 30, 2017 •
edited
Loading

@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
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 approved these changes Jul 11, 2017
View reviewed changes
Copy link
Contributor
@PaulHigin PaulHigin left a comment

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
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
Reviewers

@PaulHigin PaulHigin PaulHigin approved these changes

@lzybkr lzybkr Awaiting requested review from lzybkr

Assignees

@daxian-dbw daxian-dbw

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@oising @iSazonov @daxian-dbw @PaulHigin @msftclas
0