Supporting namespaces in partial classes #149
Conversation
They didn't check for namespace - it started missing
RunninglVlan
changed the title
Getting CurrentDirectory instead of hard-coded path
When collecting file types - Those inner types will be merged as part of partial class
(cherry picked from commit da6a48d6711c5ef5d5b0698c085c9568dbfe5361)
|
that's the spirit! I'll check it out, thanks! |
| var combinedUsingDirectives = new HashSet<UsingDirectiveSyntax>(); | ||
| var combinedTypesDefined = new HashSet<string>(); | ||
| // root key is namespace, inner key is type | ||
| var combinedTypes = new Dictionary<string, Dictionary<string, List<TypeDeclarationSyntax>>>(); |
There was a problem hiding this comment.
would be good to use named type - this way comment is not necessary and whoever works with the structure knows what it represents
There was a problem hiding this comment.
So how would you like this type to look like? Anyway, feel free to make it a type ;)
| continue; | ||
| } | ||
|
|
||
| var namespaceName = NamespaceHelper.GetNamespaceName(typeDecl); |
There was a problem hiding this comment.
why did we previously need FQDN and now we just use namespace instead? Does that not break some other use cases?
There was a problem hiding this comment.
Type name is still used below. We're grouping namespaces and types in those namespaces.
There was a problem hiding this comment.
Alright just to clarify my question some more - FQDN would suggest like RootNamespace.ChildNamespace - so to me GetNamespaceFQDNName would return whole string where GetNamespaceName would return just ChildNamespace - from code it looks that's the case. I'm just wondering if that change doesn't break previous code that assumed it'll be FQND?
There was a problem hiding this comment.
Previously it was returning full namespace + type name, now it returns just full namespace.
I just tested and it works as I said.
Assets/Scripts/Editor/Compilation/CodeRewriting/Partials/SyntaxTreeMergePartialsExtensions.cs
Show resolved
Hide resolved
Assets/Tests/Editor/Integration/CodePatterns/CompileWithRedirectTestBase.cs
Show resolved
Hide resolved
|
Nice! Some good stuff here, thanks for the changes. I left some notes - I think we should change it a little bit so it's easier to understand. Also flagging @Jlabarca as he made partials support intially and may be able to see something I don't with regards to existing functionality |
I got tired of waiting and fixed #137 myself 😅
Also included fix for tests - all of them are now green on my machine too.
And small performance improvement.
Found another bug and fixed it - partial type inner types were duplicated on namespace level (there was correct inner type inside partial type and the same was duplicated on namespace level and resulting file wouldn't compile).
And another bug and fixed it - using directives were duplicated in merged partial classes.