-
Notifications
You must be signed in to change notification settings - Fork 14
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
Enable internal classes #43
base: master
Are you sure you want to change the base?
Conversation
I like it, can you do a few things:
Thanks! |
Sure thing |
So, working on the broader topic, what would we want to have?
Wrt. nested members:
What are your thoughts on that? |
Sorry, I was occupied :( |
b12d15b
to
68f191c
Compare
68f191c
to
54eac72
Compare
Tests split, version bumped :) |
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.
Overall I like it.
But it does not yet touch nested classes if I see it correctly?
@@ -25,7 +25,7 @@ | |||
<PackageLicenseExpression>MIT</PackageLicenseExpression> | |||
<EnableNETAnalyzers>True</EnableNETAnalyzers> | |||
<AnalysisLevel>latest-Recommended</AnalysisLevel> | |||
<Version>2.3.0</Version> | |||
<Version>3.0.0</Version> |
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.
Yes, definitely needed.
I would suggest also adding examples, changelog and your name to Readme.md - especially calling out that you fixed a bug where classes without access modifier where wrongly set to public instead of internal.
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.
Good point!
|
||
public partial class AccessibilityTests | ||
{ | ||
private static string GenerateCode(string code) |
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.
Hm, this dublicates code in the original tests.
Maybe use the same partial class tests I've used like in AutomaticInterface/Tests/GeneratorsTests.MethodParameters.cs?
} | ||
|
||
[Fact] | ||
public void AddsPublicMethodToInterface() |
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.
What does this test test?
Seems to me already tested by existing tests?
} | ||
|
||
[Fact] | ||
public void WorksWithInternal() |
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.
I don't see internal here?
} | ||
|
||
[Fact] | ||
public void WorksWithDefaultAccessModifier() |
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.
Looks like the above test?
Sorry, had several things on my plate those last few months. Still interested? |
Yes! You need probably to rebase |
To facilitate API design for assemblies, [GenerateAutomaticInterface] should take
internal
visibility into account. The interface can be useful for dependency injection without necessarily being part of the public API of an assembly.