Skip to content

Convert PlayerChooseInitialServerEvent into a resulted event#1787

Draft
electronicboy wants to merge 2 commits intodev/3.0.0from
cat/initial-choose-server-resulted
Draft

Convert PlayerChooseInitialServerEvent into a resulted event#1787
electronicboy wants to merge 2 commits intodev/3.0.0from
cat/initial-choose-server-resulted

Conversation

@electronicboy
Copy link
Copy Markdown
Member

No description provided.

Component.translatable("velocity.error.no-available-servers", NamedTextColor.RED)),
true);
//noinspection UnnecessaryReturnStatement
return;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

//noinspection UnnecessaryReturnStatement
return;

Seems unnecessary? I don't see why you would want a return here?
Maybe one of those fancy new switch "instanceof" statements would be better here anyway (JEP 441), throwing on default (unknown/unexpected result type)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'd originally wrote this without the else statement and then remembered I needed it so I could for the if anyways. I do know that IJ itself tended to get upset with shorter case switches and so I did tend to avoid them for stuff like this

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fair enough, still I think line 270 and 271 can be deleted

}

public record Denied(@Nullable Component reason) implements Result {
public static @NotNull Result of(@Nullable Component reason) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Inconsistent formatting (no whitespace after class declaration, Allowed does have this)

Is it an idea to force this with checkstyle? Whitespace after class declaration, or newline checking in general, idk if we currently have this. Iirc this is quite inconsistent in the codebase

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yea, we don't really have a hard rule on this and it's pretty easy to miss when refactoring during working on something, need to update checkstyle and formalise rules for this stuff

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.

2 participants