Skip to content

Drop "Enum" prefix for enumerations #815

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

Closed
kashike opened this issue Apr 25, 2019 · 33 comments
Closed

Drop "Enum" prefix for enumerations #815

kashike opened this issue Apr 25, 2019 · 33 comments
Assignees
Milestone

Comments

@kashike
Copy link
Collaborator

kashike commented Apr 25, 2019

We're planning to do the following changes to make things consistent, and 1.14 is a good time to do it with the large changes.

Do note that these are only what vanilla names will use - you're free to name your own classes in your mods however you like.


Propsed change

  • Drop the Enum prefix for enumerations
    • this is a hold over from back before packages were a thing with MCP - we essentially faked packages by grouping class names together with prefixes
    • we don't prefix concrete classes with C, etc

Please react with 👍 if you are in favour, and 👎 if you are not in favour.

It it HIGHLY encouraged that you read all responses to this, you can change your vote at any time up until the deadline. Please take this seriously as all results are final after the deadline.

Deadline for feedback and voting is Noon PDT Monday (29/04/2019).

ref #814

@kashike kashike added this to the 1.14.0 milestone Apr 25, 2019
@kashike kashike self-assigned this Apr 25, 2019
@kashike kashike changed the title Drop "Enum" prefix for interfaces Drop "Enum" prefix for enumerations Apr 25, 2019
@DarkGuardsman
Copy link

Does someone have a list of all the Enums so I can check if they conflict with any of my APIs when Enum is removed.

@ChloeDawn
Copy link

How would they conflict? They'd be in different classpaths unless you're storing your API classes in the minecraft packages...

@bs2609
Copy link
Contributor

bs2609 commented Apr 25, 2019

All enums are direct subclasses of the Enum class, and can be found by an appropriately-scoped type hierarchy lookup.

@DarkGuardsman
Copy link

Conflict as in imports, so for example if I had a Facing enum that existed in previous code with EnumFacing. Then we dropped Enum it would mean that some classes would two Facing imports. I would like to correct this ahead of the switch.

@Pokechu22
Copy link

Ignoring inner classes of the enums, but allowing for enums declared inside another class:

22:55:47     pokechu22 | findallc ^(.+\$)?Enum[^$]+$
22:55:47 MCPBot_Reborn | +++ CLASSES +++
22:55:47 MCPBot_Reborn | bcs$b => net/minecraft/block/Block$EnumOffsetType
22:55:47 MCPBot_Reborn | bzc => net/minecraft/block/material/EnumPushReaction
22:55:47 MCPBot_Reborn | cuw$a => net/minecraft/client/renderer/BlockModelRenderer$EnumNeighborInfo
22:55:47 MCPBot_Reborn | ctw => net/minecraft/client/renderer/EnumFaceDirection
22:55:47 MCPBot_Reborn | ddx$a => net/minecraft/client/renderer/vertex/VertexFormatElement$EnumType
22:55:47 MCPBot_Reborn | ddx$b => net/minecraft/client/renderer/vertex/VertexFormatElement$EnumUsage
22:55:47 MCPBot_Reborn | awf => net/minecraft/enchantment/EnumEnchantmentType
22:55:47 MCPBot_Reborn | afc => net/minecraft/entity/EnumCreatureType
22:55:47 MCPBot_Reborn | aog$b => net/minecraft/entity/player/EntityPlayer$EnumChatVisibility
22:55:47 MCPBot_Reborn | aoh => net/minecraft/entity/player/EnumPlayerModelParts
22:55:47 MCPBot_Reborn | auo => net/minecraft/item/EnumAction
22:55:48 MCPBot_Reborn | asc => net/minecraft/item/EnumDyeColor
22:55:48 MCPBot_Reborn | atq => net/minecraft/item/EnumRarity
22:55:48 MCPBot_Reborn | hx => net/minecraft/network/EnumConnectionState
22:55:48 MCPBot_Reborn | iw => net/minecraft/network/EnumPacketDirection
22:55:48 MCPBot_Reborn | kt$a => net/minecraft/network/play/server/SPacketPlayerPosLook$EnumFlags
22:55:48 MCPBot_Reborn | cfe$b => net/minecraft/scoreboard/Team$EnumVisible
22:55:48 MCPBot_Reborn | bmh => net/minecraft/state/EnumProperty
22:55:48 MCPBot_Reborn | adm => net/minecraft/util/EnumActionResult
22:55:48 MCPBot_Reborn | bgy => net/minecraft/util/EnumBlockRenderType
22:55:48 MCPBot_Reborn | er => net/minecraft/util/EnumDirection8
22:55:48 MCPBot_Reborn | eq => net/minecraft/util/EnumFacing
22:55:48 MCPBot_Reborn | adk => net/minecraft/util/EnumHand
22:55:48 MCPBot_Reborn | aez => net/minecraft/util/EnumHandSide
22:55:48 MCPBot_Reborn | xp => net/minecraft/util/EnumTypeAdapterFactory
22:55:48 MCPBot_Reborn | k$b => net/minecraft/util/Util$EnumOS
22:55:48 MCPBot_Reborn | adi => net/minecraft/world/EnumDifficulty
22:55:48 MCPBot_Reborn | ayi => net/minecraft/world/EnumLightType
22:55:48 MCPBot_Reborn | bmu => net/minecraft/world/border/EnumBorderStatus
22:55:48 MCPBot_Reborn | bnj$a => net/minecraft/world/chunk/Chunk$EnumCreateEntityType

@bs2609
Copy link
Contributor

bs2609 commented Apr 25, 2019

Of course, as mentioned in #817, not all of those are actually enums (e.g. EnumProperty, EnumTypeAdapterFactory).

@liach
Copy link

liach commented Apr 26, 2019

Will enums be suffixed instead? for example, alibaba suffixes enum classes with Enum suffix to prevent confusion https://github.com/alibaba/Alibaba-Java-Coding-Guidelines#naming-conventions rule 14

@kashike
Copy link
Collaborator Author

kashike commented Apr 26, 2019

No, enums will not be suffixed.

@liach
Copy link

liach commented Apr 26, 2019

As long as enum names won't conflict with concrete class names (like one of the concerns in #816), I support this.

@JBYoshi
Copy link

JBYoshi commented Apr 26, 2019

After running a program through the Minecraft class files, the only enum name conflict is ActionResult/EnumActionResult. The enum can probably be renamed to ActionResultType.

@Darkhax
Copy link

Darkhax commented Apr 26, 2019

I was under the impression that this issue had already been decided on ages ago? (The decision was to remove Enum prefix)

@Pokechu22
Copy link

I believe that a while ago it was decided that new enums should be named without the prefix, but this is about removing it on existing ones. Though I'm not completely sure about that.

@Sollace
Copy link

Sollace commented Apr 26, 2019

@kashike To add a bit to this: We also get a lot of things like BlockFlower.EnumFlowerType. In these cases could we shorten these to simply BlockFlower.Type and encourage more qualified names?

@DarkGuardsman
Copy link

@Darkhax ya I'm confused by this as well, we have had this conversation several times before. Out of all 3 changes the Enum one is the most accepted. As EnumFacing -> Facing or even better FacingDirection is a solid change. Since enums are used a data types rather than being thought of as classes or interfaces.

Though I guess I would be contradicting myself with my argument for "I" as "Enum" does the same goal. Its a complex subject as better naming for enums is needed. However, this has the same side effect of churn and invalidation of tutorials. If we are going to change the enums we should consider renaming the class to be a bit stronger. So instead of Facing we end up with FacingDirection or BlockFace or something that makes it a bit easier to understand the context. As Facing will be very generic and result in the question "Facing of what? Facing in, facing out, facing how?"

@Sollace
Copy link

Sollace commented Apr 26, 2019

@DarkGuardsman How many different types of facings do you think there are?

I'm aware of one: EnumFacing.

There is a class dealing with the direction a dispenser/dropper faces, but that's not an enum and it can be easily named DispenserDirection

And if a block introduces its own enum for facing you always have FenceBlock.Facing to distinguish from any others.

@DarkGuardsman
Copy link

Not sure why your asking that question. As I'm just using EnumFacing as an example since its name can be confusing at times. Even back when we had ForgeDirection it was never really clear on its meaning/usage. Often developers get confused on if the face is in towards the block or away from the block. They also sometimes confuse if its relative to the block or the world. Fixing the naming can help with this in some cases. Not all as a lot of that is just normal dev confusion.

This likely holds true for other enums such as EnumHand, EnumAction, EnumRarity, etc. As each does not give a context clue of what it relates towards. Is it the Hand of the player, zombie, any biped, can it be used for a pig, ghast, etc? Obviously its a biped but that name doesn't relate clearly towards that meaning. What I'm suggest is if we bit the bullet on removing Enum can we also do it for the name as a whole.

@LexManos
Copy link
Member

It may of been discussed before, but that did not involve old names. And we have not built/documented a spec to follow.
This is what these issues are for.
This one is fairly universally supported, so this particular issue is just documentation.

As for Inner enums, There are already quite a few that have generic 'X.Type' names, and I see nothing wrong with that. So yes, BlockFlower.EnumFlowerType -> BlockFlower.Type.

@liach
Copy link

liach commented Apr 26, 2019

@kashike mind closing #717?

@Sollace
Copy link

Sollace commented Apr 26, 2019

@DarkGuardsman I just thought you were implying that there are a lot of other confusing enums of that nature.

In the matter of EnumFacing: Yeah, I agree, kinda. To me EnumFacing has always just been that, a facing. How it's used varies quite a bit, so in the case of most blocks it's which direction the "front/face" of a block is directed. In others it's used to work with the world's coordinate system, as in the Axis, Planes, and etc.

In some forge methods they're related to specific faces of blocks.

(never heard of the in-towards case as everything I've seen so far has been relative to the direction a block's face, um, faces relative to the world's directions cardinal)

They just seem to have a wide breadth of uses depending on the context, which I think is okay if not a little confusing. It just means we have to be a bit more explicit about the context in which we're using them. So yeah, as Lex said, documentation.

EnumHand

Funny you mention that, I've seen somewhere another odd duplication. Something like EnumHand and EnumHandSide.

Former is obviously relating to the hand slot (EnumHand -> HeldSlot, MAIN_HAND -> PRIMARY, OFF_HAND -> SECONDARY) of an entity. The other one seems to relate more to the entity's natural handedness (EnumHandSide -> Handedness).

@liach
Copy link

liach commented Apr 26, 2019

We can change EnumHand to DualWielding (which is the official term on mc wiki). For hand side, handedness sounds great

@DarkGuardsman
Copy link

@liach if anything it should be BipedHand, since its mainly used for anything that has 2 hands.

@Sollace The in-facing problem was more of an issue with ForgeDirection back before it was well documented and defined. We use to have APIs that used it differently based on their own needs. So you could have 1 API expect out facing side and another in-facing side. Most of us eventually sorted it out but it was a problem for a solid few years.

Either way this is closed, maybe we should create another for talking about name changes in general for some classes?

@Pokechu22
Copy link

Most of those issues would also be resolved if #589 were fixed - documentation making it explicit which way a facing works would be far more helpful than any of the possible names.

@LexManos
Copy link
Member

Yes, but that requires major rewrite of the MCP database. That's another project that is on the TODO and we can talk about if people want to help do it.

@DarkGuardsman
Copy link

Yep, and I think the current documentation has solved this mostly. I brought it up as an example of what problems are faced with poorly named things. As for help let me know what you need.

@thiakil
Copy link

thiakil commented Apr 27, 2019

@Sollace the enum constants (e.g. MAIN_HAND) are the mojang values from the bytecodes, and shouldn't be changed otherwise you'd end up with EnumHand.valueOf("MAIN_HAND") == EnumHand.PRIMARY & EnumHand.PRIMARY.name() == "MAIN_HAND"

@AlexModGuy
Copy link

AlexModGuy commented Apr 28, 2019

Will this just be Enums or will it eventually be other classes ex. EntityOcelot -> Ocelot or OcelotEntity?
EDIT: nvm im an idiot #817

@Sollace
Copy link

Sollace commented Apr 28, 2019

@thiakil

@Sollace the enum constants (e.g. MAIN_HAND) are the mojang values from the bytecodes, and shouldn't be changed otherwise you'd end up with EnumHand.valueOf("MAIN_HAND") == EnumHand.PRIMARY & EnumHand.PRIMARY.name() == "MAIN_HAND"

Oh, doi

Of course. I've always thought enum constants were obfuscated as well. A lot of the code I see in the game has situations where they explicitly specify an alternate name to use, so I figured that was to get around obfuscation shenanigans.

In that case, yeah, I agree. The names shouldn't be changed.

if anything it should be BipedHand, since its mainly used for anything that has 2 hands.

Someone would have to corroborate that. I'd thought I'd seen instances where it was being used on things other than bipedal entities, or rather the code that deals with it is actually common to all living entities.

@liach

Either way this is closed, maybe we should create another for talking about name changes in general for some classes?

Yes, please. If there's anything more to add, can we put it into a new issue?

@liach
Copy link

liach commented Apr 28, 2019

@thiakil
Copy link

thiakil commented Apr 28, 2019

I've always thought enum constants were obfuscated as well.

@Sollace the compiled public static field names are, but in compiled code enums have 2 extra constructor params, because they call java.lang.Enum's constructor with them (the string name and the ordinal)

@wanderingmage
Copy link

I say whatever convention makes reading the code easier.

@ModCoderPack ModCoderPack locked as resolved and limited conversation to collaborators Apr 29, 2019
@kashike
Copy link
Collaborator Author

kashike commented Apr 29, 2019

Thank you for the input, everyone. This issue is now closed for voting, and the results will be provided shortly.

@kashike
Copy link
Collaborator Author

kashike commented Apr 29, 2019

Result: Favor
=====================================
280: Voted in favor.
43: Voted in opposition.
0: Are indifferent
323: Total votes.
3: Accounts less than a week old.
=====================================

Results calculated using https://github.com/illyohs/VoteChecker

@LexManos
Copy link
Member

LexManos commented May 9, 2019

See full announcement here and related name changes here

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

No branches or pull requests