Skip to content

Address some stuff reported/suggested by code analyzers #167

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Saancreed
Copy link
Member

Lots of more or less stupid changes to make the .NET analyzers happy. I know some of them might be controversial so feel free to fight me where appropriate 🥊

return type.GetCustomAttributes<AuthorizeWhenAttribute>().Select(AuthorizerDefinition.Create).ToList();
}
public static List<AuthorizerDefinition> GetCustomAuthorizers(Type type) =>
[.. type.GetCustomAttributes<AuthorizeWhenAttribute>().Select(AuthorizerDefinition.Create)];
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a fan of this syntax. Also this space after .. is weird for me, but that's probably a csharpier thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

The space is indeed a CSharpier thing. Weird, but I don't hate it.

(Take a look at how CSharpier formats XML files like csprojs to see something truly cursed 🙈)

As for [.. foo] vs foo.ToBar(), I'm not opposed to keeping the latter but the former does have one major advantage: it does the right thing with a single syntax, for many collection types, as long as the type can be inferred from context. But maybe there's enough space to keep both of them, without favoring one over another?

Copy link
Contributor

Choose a reason for hiding this comment

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

Take a look at how CSharpier formats XML files like csprojs to see something truly cursed

What do you mean? I've used csharpier 1.0.x somewhere, and csprojs seemed fine? 🤨

I'm not opposed to keeping the latter but the former does have one major advantage: it does the right thing with a single syntax, for many collection types

Right, that's true. I'm not hugely opposed, might even convience myself to using it, so there's place for coexistence of both for sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean? I've used csharpier 1.0.x somewhere, and csprojs seemed fine? 🤨

<TargetFrameworks Condition="$([MSBuild]::VersionLessThan('$(NETCoreAppMaximumVersion)', '9.0'))"
>net8.0</TargetFrameworks
>

Comment on lines 15 to +23
[Theory]
[InlineData((byte)10)]
[InlineData((sbyte)10)]
[InlineData((int)10)]
[InlineData((long)10)]
[InlineData((short)10)]
[InlineData((uint)10)]
[InlineData((ulong)10)]
[InlineData((ushort)10)]
[InlineData(10)]
[InlineData(10L)]
[InlineData(10U)]
[InlineData(10UL)]
Copy link
Contributor

Choose a reason for hiding this comment

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

The old one was consistent at least.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, except .NET complained that the cast in (int)10 was redundant. If only we had byte and short literals 😢

Comment on lines +8 to +12
public class ContractsGenerator(CompiledContracts contracts)
{
private readonly CompiledContracts contracts;
private readonly CompiledContracts contracts = contracts;

private readonly TypeRefFactory typeRef;

public ContractsGenerator(CompiledContracts contracts)
{
this.contracts = contracts;

typeRef = new(contracts);
}
private readonly TypeRefFactory typeRef = new(contracts);
Copy link
Member

Choose a reason for hiding this comment

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

Arguably this should not be changed to a primary constructor

Update System.Collections.Immutable to v9 and use FrozenSet.Create
Copy link

Test Results

  2 files  ±0    2 suites  ±0   45s ⏱️ ±0s
 82 tests ±0   82 ✅ ±0  0 💤 ±0  0 ❌ ±0 
178 runs  ±0  178 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit ec3e2a9. ± Comparison against base commit cf1ac68.

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.

3 participants