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

Rename SIDEBAR_TEAM_NO_COLOR to SIDEBAR, allow getting DisplaySlot by team colour #2350

Merged
merged 2 commits into from
May 14, 2021

Conversation

dualspiral
Copy link
Contributor

@dualspiral dualspiral commented May 10, 2021

SpongeAPI | Sponge

This superscedes #2295 (thanks to @thibaulthenry for the initial commit!)

Makes withTeamColor static and renames it to findByTeamColor. I have used a factory to provide the mapping from colour to slot because I couldn't really think of any logical place to put an instance method that does this mapping (maybe Scoreboard could house such a method, but it felt a little out of place).

Are we okay with this being a factory, or should we look to make this an instance method somewhere?

@dualspiral dualspiral added status: input wanted system: scoreboard api: 8 (u) version: 1.16 (unsupported since Oct 17th 2023) labels May 10, 2021
@Zidane
Copy link
Member

Zidane commented May 10, 2021

Yeah that is an odd name isn't it. That isn't a factory. It doesn't create instances, correct?

How does that actually work, in concept, when you don't provide a Scoreboard to query?

@dualspiral
Copy link
Contributor Author

Yeah that is an odd name isn't it. That isn't a factory. It doesn't create instances, correct?

That's right - which is why I'm icky about it, it just doesn't feel right.

How does that actually work, in concept, when you don't provide a Scoreboard to query?

Not sure I'm 100% sure what you mean here, but all a display slot is in our impl is something that wraps around an integer. That integer is used to get an element from an Objectives array. So, for all intents and purposes, a DisplaySlot is just a fancy way to abstract the magic numbers away - no connection to a scoreboard what so ever.

@kashike
Copy link
Contributor

kashike commented May 10, 2021

A factory doesn't always need to create instances, technically - it can simply provide already-created things too.

@Zidane
Copy link
Member

Zidane commented May 10, 2021

@dualspiral

Ah that makes sense, thanks for the clarification. You are also correct @kashike , based on that terminology that works here. This can remain then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: 8 (u) version: 1.16 (unsupported since Oct 17th 2023) status: input wanted system: scoreboard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants