Skip to content
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

Emit new() type constraints on generic type parameters; emit params keyword for method parameters #65

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

simonmckenzie
Copy link
Contributor

@simonmckenzie simonmckenzie commented Dec 3, 2024

This addresses #64, where type parameters weren't having the new() constraint applied.

I have also addressed a comment by @ChaseFlorell regarding missing params keywords.
#62 (comment)

Finally, I have updated some tests that were failing due to changes in the inheritdoc annotations and re-ran csharpier, as some existing files were failing linting.

This addresses codecentric#64, where type parameters weren't having the `new()` constraint applied.

Also updated some tests that were failing due to changes in the `inheritdoc` annotations and re-ran csharpier.
&& x.AttributeClass
.Name
.Contains(AutomaticInterfaceGenerator.DefaultAttributeName)
.FirstOrDefault(x =>
Copy link
Contributor Author

@simonmckenzie simonmckenzie Dec 3, 2024

Choose a reason for hiding this comment

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

Csharpier reformat - this was failing on master.

@@ -1820,10 +1821,10 @@ namespace AutomaticInterfaceExample
[global::System.CodeDom.Compiler.GeneratedCode("AutomaticInterface", "")]
public partial interface IDemoClass
{
/// <inheritdoc />
/// <inheritdoc cref="AutomaticInterfaceExample.DemoClass.AMethod(Func{Task{int}})" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests were failing on master. Updated.

@simonmckenzie simonmckenzie force-pushed the fix/add-new-type-constraint-to-generated-interface-methods branch from 5990dd0 to 3b91fd9 Compare December 4, 2024 00:26
@simonmckenzie simonmckenzie changed the title Add the new() type constraint to method parameters Emit new() type constraints on generic type parameters; emit params keyword for method parameters Dec 4, 2024
Small fix - the existing code wasn't emitting the "params" keyword
@simonmckenzie simonmckenzie force-pushed the fix/add-new-type-constraint-to-generated-interface-methods branch from 3b91fd9 to b978bae Compare December 4, 2024 00:29
@simonmckenzie
Copy link
Contributor Author

I hope it's OK that I have fixed two things in one PR here @ChristianSauer - it just felt it'd be easier since the master pipeline was already failing, and both would have needed the same set of base fixes (linting and rehabilitation of broken tests).

…tring(FullyQualifiedDisplayFormat)`

This removes a fair bit of code and ensures we're formatting our parameters consistently.
@@ -105,7 +105,10 @@ private static void AddMethod(InterfaceBuilder codeGenerator, IMethodSymbol meth
ActivateNullableIfNeeded(codeGenerator, method);

var paramResult = new HashSet<string>();
method.Parameters.Select(GetMethodSignature).ToList().ForEach(x => paramResult.Add(x));
method
.Parameters.Select(x => x.ToDisplayString(FullyQualifiedDisplayFormat))
Copy link
Contributor Author

@simonmckenzie simonmckenzie Dec 4, 2024

Choose a reason for hiding this comment

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

The FullyQualifiedDisplayFormat can format the parameters for us, so we can now remove all the custom code for generating parameter signatures (see the deletions of the GetMethod... methods below)

Comment on lines +20 to +22
| SymbolDisplayParameterOptions.IncludeParamsRefOut
| SymbolDisplayParameterOptions.IncludeDefaultValue
| SymbolDisplayParameterOptions.IncludeName,
Copy link
Contributor Author

@simonmckenzie simonmckenzie Dec 4, 2024

Choose a reason for hiding this comment

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

I've expanded the parameter formatting rules so we can use this formatter for parameter signature generation.

typeQualificationStyle: SymbolDisplayTypeQualificationStyle.NameAndContainingTypesAndNamespaces,
globalNamespaceStyle: SymbolDisplayGlobalNamespaceStyle.Included,
miscellaneousOptions: SymbolDisplayMiscellaneousOptions.UseSpecialTypes
| SymbolDisplayMiscellaneousOptions.IncludeNullableReferenceTypeModifier
| SymbolDisplayMiscellaneousOptions.EscapeKeywordIdentifiers
Copy link
Contributor Author

@simonmckenzie simonmckenzie Dec 4, 2024

Choose a reason for hiding this comment

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

This ensures that parameters that share names with keywords will be escaped.

This replicates the behaviour of the (now deleted) GetMethodSignature method.

(there's an existing test for this, WorksWithReservedNames).

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.

1 participant