8000 Add 'CSharpVersion7' member into public 'Language' enum by iSazonov · Pull Request #3933 · PowerShell/PowerShell · GitHub
[go: up one dir, main page]

Skip to content

Add 'CSharpVersion7' member into public 'Language' enum #3933

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

Merged
merged 1 commit into from
Jun 20, 2017

Conversation

iSazonov
Copy link
Collaborator
@iSazonov iSazonov commented Jun 5, 2017

Close #3853

Public enum 'LanguageVersion' from 'Microsoft.CodeAnalysis.CSharp'
already contains 'CSharp7' member.
After moving PowerShell to .Net Core 2.0 we should add 'CSharpVersion7'
member into public 'Microsoft.PowerShell.Commands.Language' enum too.

This will allow 'Add-Type -Language CSharpVersion7'

Public enum 'LanguageVersion' from 'Microsoft.CodeAnalysis.CSharp'
already contains 'CSharp7' member.
After moving PowerShell to .Net Core 2.0 we should add 'CSharpVersion7'
member into public 'Microsoft.PowerShell.Commands.Language' enum too.

This will allow 'Add-Type -Language CSharpVersion7'
It "Public 'Language' enumeration contains all members" {
[Enum]::GetNames("Microsoft.PowerShell.Commands.Language") -join "," | Should Be "CSharp,CSharpVersion7,CSharpVersion6,CSharpVersion5,CSharpVersion4,CSharpVersion3,CSharpVersion2,CSharpVersion1,VisualBasic,JScript"
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a test case for 'Add-Type -Language CSharpVersion7'?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, should there be two commits (one for the product change and one for the test)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We checked it manually in the original Issue.
I didn't touch the functionality of this cmdlet. Besides, we don't even have tests for it right now. So I believe this is a separate huge job.
It seems @daxian-dbw is working on Add-Type. It would be good to agree on the distribution of work on tests.

I can say nothing about separate commits. 😕

Copy link
Member

Choose a reason for hiding this comment

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

Two commits (one for code changes and one for tests) is not a strict requirement.

@mirichmo mirichmo self-assigned this Jun 7, 2017
@iSazonov
Copy link
Collaborator Author

@mirichmo Could you please continue with the PR?

@mirichmo mirichmo merged commit f30b06f into PowerShell:master Jun 20, 2017
@iSazonov iSazonov deleted the CSharpVersion7 branch June 20, 2017 03:00
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.

4 participants
0