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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix recent collection initialiser inspection #27167

Merged
merged 1 commit into from Feb 14, 2024

Conversation

smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented Feb 14, 2024

As seen in https://github.com/ppy/osu/actions/runs/7896613956/job/21550875129?pr=27147

This appears to have come up because .NET 8.0.200 has been made available to the CI runner, and we've only been using .100.

It looks like what the inspection wants us to do here is something like...

new RulesetSkinProvidingContainer(...)
{
    (skinRequester = new ...)
};

Which is pretty 馃ぎ

@smoogipoo smoogipoo requested a review from bdach February 14, 2024 08:23
@smoogipoo smoogipoo changed the title Prefer collection initializer over collection expression syntax Fix new collection initialiser inspection Feb 14, 2024
@smoogipoo smoogipoo changed the title Fix new collection initialiser inspection Fix recent collection initialiser inspection Feb 14, 2024
@smoogipoo smoogipoo merged commit 3b8203a into ppy:master Feb 14, 2024
15 of 17 checks passed
Child = rulesetSkinProvider;
Child = new RulesetSkinProvidingContainer(Ruleset.Value.CreateInstance(), Beatmap.Value.Beatmap, Beatmap.Value.Skin)
{
requester
Copy link
Collaborator

@bdach bdach Feb 14, 2024

Choose a reason for hiding this comment

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

Was in the middle of leaving review on this.................

Feeling pretty bad about this. The fact that this is a framework container using C# collection initialiser syntax feels very footgunny.

Like the usual idiomatic way to write this is Child = requester, no?

What was wrong with the editorconfig suppression that was here before?

Copy link
Contributor Author

@smoogipoo smoogipoo Feb 15, 2024

Choose a reason for hiding this comment

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

Ah yeah. I wanted to get this PR out of the way because it was blocking me from looking at/merging others.

I've pushed the fix straight to master: a037dbf

What was wrong with the editorconfig suppression that was here before?

Doesn't seem necessary after all (and doesn't fix this issue, because it still wants to use the initializer). The issue is as the OP says.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants