-
Notifications
You must be signed in to change notification settings - Fork 15.6k
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
Move generated test code for Google.Protobuf.Test to a separate lib #6832
Move generated test code for Google.Protobuf.Test to a separate lib #6832
Conversation
csharp/src/Google.Protobuf.Test.CodeGen/Google.Protobuf.Test.CodeGen.csproj
Outdated
Show resolved
Hide resolved
csharp/src/Google.Protobuf.Test.CodeGen/Google.Protobuf.Test.CodeGen.csproj
Outdated
Show resolved
Hide resolved
Test failure: |
That's fixed with #6828 |
If you want I can move it to this PR |
66e3be3
to
133776b
Compare
and without the internal visibility from the test project (all of which have caused issues in the past). | ||
--> | ||
<PropertyGroup> | ||
<TargetFrameworks>net451;netstandard1.0;netstandard2.0</TargetFrameworks> |
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.
nit: just net45 instead of net451 should be enough?
csharp/src/Google.Protobuf.Test.CodeGen/Google.Protobuf.Test.CodeGen.csproj
Outdated
Show resolved
Hide resolved
Looks like C#3 is nearly not enough. I think we can set the LangVersion to 6 in this PR, leave a TODO and take a followup action to make it compatible with C#3 again in a future PR?
|
I could also change it in this PR (or another PR and rebase), since it's a pretty simple change. private ExtensionSet<T> _Extensions => _extensions; to private ExtensionSet<T> _Extensions { get { return _extensions; } } |
133776b
to
16578f9
Compare
<Project Sdk="Microsoft.NET.Sdk"> | ||
|
||
<!-- | ||
This CodeGen project is kept seperate from the original test project for many reasons. |
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.
nit: still says "CodeGen" after renaming to TestProtos
typo: s/seperate/separate/
Can we make that change in a separate PR and only use this PR to verify the end result? - It makes things easier to review. |
I merged #6856, can you update this PR? |
Set lang version to C# 3.0 Add .NET Standard 2.0 target Fix partial diagnostic string since it requires C# 6
2e1bc47
to
51fa766
Compare
Rebased 👍 @jtattermusch |
Missed a few things when renaming project. Should've gotten everything now. |
Distcheck is failing, but otherwise looking good.
|
Done @jtattermusch |
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.
LGTM.
Many of the recent issues with proto2 extensions were accidentally caused by tests passing when code-gen didn't actually compile outside of internal visibility which the Google.Protobuf.Test project has. To prevent mistakes like this in the future, this separates the code-gen from the test project to:
InternalAccessVisibleTo
attribute out of the test code-gen/cc: @jtattermusch