-
Notifications
You must be signed in to change notification settings - Fork 374
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
Source generator for better access of members from inherited components #1272
base: develop
Are you sure you want to change the base?
Conversation
…from inherited components
Could you improve this to use |
source/Nuke.SourceGenerators/AccessInheritanceAsMembersGenerator.cs
Outdated
Show resolved
Hide resolved
source/Nuke.SourceGenerators/AccessInheritanceAsMembersGenerator.cs
Outdated
Show resolved
Hide resolved
continue; | ||
|
||
// must be partial | ||
if (!type.DeclaringSyntaxReferences.Any( |
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.
Although build projects are probably small in size, GetSyntax
is potentially expensive. Couldn't an optimization just check for DeclaringSyntaxReferences.Count
to be greater 1 ?
foreach (var type in allTypes) | ||
{ | ||
// must be a class | ||
if (type.TypeKind != TypeKind.Class) |
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'd move out all the filtering (class, INukeBuild
, partial) to a relevantTypes
variable.
|
||
internal partial class Build | ||
{ | ||
protected IDisposable IDisposable => this; |
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.
Not clear to me why IDisposable
should show up here. I'd filter to interfaces that implement INukeBuild
.
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.
Maybe I didn't understood the issue correctly.
I thought each class (like Build) that inherits from INukeBuild should have all interfaces (no matter what they are inheriting) as members.
But maybe what you are expecting is that the IHazRepository of the example inherits INukeBuild and that is the reason, why IHazRepository should be a member of the Build class.
If this is the case, this wasn't clear to me, as I thought the INukeBuild is a marker for the Build class.
Maybe, you can extend the example and the description a little bit :-)
Left a couple of comments. Sorry, it's not as easy/straightforward as I thought (considering I added the |
No problem, I will have a look in the next view days :-) |
0231425
to
d806295
Compare
7066bbe
to
985de83
Compare
0719711
to
6fe1e3b
Compare
6d02194
to
4bfee84
Compare
85998da
to
01b2b6e
Compare
e652c9a
to
6ed9e4d
Compare
df620ac
to
482f5ac
Compare
A new source generator generates a partial class with all the inherited interfaces for a INukeBuild class.
The generator searches for all desendants from INukeBuilder that are:
The generator creates a new partial class with all the interfaces (except INukeBuild) as property and respects the accessibility.
I confirm that the pull-request: