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

Migrate from prefixes to suffixes #817

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

Migrate from prefixes to suffixes #817

kashike opened this issue Apr 25, 2019 · 61 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

  • Migrate from prefixes to suffixes for everything: FooBlock, FooItem, etc
    • not everything with a Block prefix is actually a block (applies elsewhere too):
      • BlockMatcher is actually a predicate
      • BlockPattern is actually a pattern that can be matched in the world
      • BlockState is actually a state of a block
    • doesn't apply to vanilla, but applies to mods who don't use packages and follow vanilla's class naming: FooBlock, FooItem are both grouped together, rather than BlockFoo [... many others ...] ItemFoo
      • while vanilla separates blocks and items into their own packages, a mod is not required to do the same
    • note that we already use suffixes for major refactors and new classes: see Biome and Feature classes for an example
    • Historical Note: 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

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

@williewillus
Copy link
Contributor

williewillus commented Apr 25, 2019

point: upends naming of everything in the community for little benefit. I don't particularly feel strongly for prefixes and would have abstained, but I despise needless churn in an already churny modding scene (thanks to vanilla).

there's bigger fish to fry w.r.t class names (Dimension vs DimensionType, WorldInfo, ...) than to worry about the block and item names, which are straightforward and easy to understand regardless of which choice we pick here.

@bs2609
Copy link
Contributor

bs2609 commented Apr 25, 2019

counterpoint: maybe people will care enough to contribute to the migration tools everyone ignores (e.g. https://github.com/MinecraftForge/Remapper)

@LexManos
Copy link
Member

As for the "bigger fish to fry" 1.14 is the time. Kashike will be curating all open issues for class renames. Go vote on those specific issues. This issue has no effect on any others except for if those are smurphed names. In which case they will be updated to follow whatever outcome this vote has.

@DarkGuardsman
Copy link

Problem with this is if you start importing classes with the same name. So say we had BlockMatcher and ItemMatcher then we dropped the prefix. This would mean we have two Matcher.class. Effectively resulting in more complex importing. Not saying this is actually the case but should be considered. As a minor change like this could result in less than minor complexity for developers.

The other issue is when dropping prefixes it can make it difficult to understand what something means. In the case again of BlockMatcher, if you call it just Matcher then what context clue does the name provide? I rather not have to leverage the IDE or the import path to understand what a class is when reading code.

The final thing, what do we gain by dropping the prefix?

@cpw
Copy link
Collaborator

cpw commented Apr 25, 2019

Not dropping the identifier @DarkGuardsman, moving it from prefix to suffix.

@LexManos
Copy link
Member

Your example of "BlockMatcher" would not fall into this change. As it is not the "Matcher Block" it is a Matcher, that matches Blocks. But yes, the proposal is not to drop the type, it's to move it. So "BlockDirt" -> "DirtBlock"

@DarkGuardsman
Copy link

Oh.. my bad, the other two were 'dropped' with my brain recovering from an 11 hour shift at work doing a deploy....

anyways still against this idea. Its going to add way too much unneeded rename churn, create a massive amount of confusion, and invalid any tutorial/documentation we currently have. As a community that already suffers from docs/tuts shortage that is very harmful.

We really need to ask what advantage does this offer to veteran developers, noob developers, and new developers.

New developers will likely gain nothing from the change. As its still a lot to learn either direction you look at it. Honestly might be better as DirtBlock is easier to say in english then BlockDirt. Though this only applies to english as other languages fair differently.

Noob developers will learn the change eventually but could be stunted in terms of growth. Since they are currently in the process of learning with tutorials. That would then need to be updated or reworked to note the massive change in naming. Remember even a simple name change to a new developer is a lot to work around when you're not use to that type of change.

Old developers might just flat out quit rather than deal with running a rename tool over each project then fixing anything missed. I fall into this last part personally as I have well over 150+ MC repos. With my current job I can't spend more than 8 hours a week developing. Given I'm an edge case due to the scale I develop mods at currently. Most developers will recover after a short delay of a day or so. However, it would have a negative impact on workflow with no future gain in productivity.

@bs2609
Copy link
Contributor

bs2609 commented Apr 25, 2019

Tutorials and the like will become invalidated by Minecraft version changes in any case - this will not be the extent of 1.13/1.14 changes.

To some extent, if there is to be a change, it's better to "get it over with" - if these changes came with the introduction of packages, it would be well in the past and current developers would not be affected by it.

@bs2609
Copy link
Contributor

bs2609 commented Apr 25, 2019

Another point that is worth mentioning is that Mojang themselves appear to use suffix naming. I personally don't feel we should be governed entirely by "official" names, but I think it's worth keeping it in mind.

@liach
Copy link

liach commented Apr 26, 2019

Intellij idea definitely supports suffix naming. I would just get suffix naming to have a easier life using intellij.

@ghost
Copy link

ghost commented Apr 26, 2019

I love naming conventions as much as the next guy because order and simplicity is above all else. However, some things here have less reason to change than others. Like BlockState for example. Yes it isn't exactly a Block but it still describes the state of one. It'd only make sense to rename it to something else if it were going to get a whole category for itself.

For instance, (this is hypothetical and does not involve real objects) you can have something like BlockProperties, BlockState, BlockType, etc and it is all describing a Block.

@bs2609
Copy link
Contributor

bs2609 commented Apr 26, 2019

The point of mentioning BlockState in the initial post is as something that will not change - it's not a block, so it will not have a Block suffix.

@thiakil
Copy link

thiakil commented Apr 26, 2019

not everything with a Block prefix is actually a block (applies elsewhere too)

This reads like it's being used as a reason for the changing, which seems odd, when you can just change those non-matching names.

@bs2609
Copy link
Contributor

bs2609 commented Apr 26, 2019

It is probably as much a reason for this change as names colliding is a reason against #815 and #816.

It would be good to hear suggestions for your proposed renames of those type of classes, so we can look to implementing those name changes instead if this is not taken forward.

@thiakil
Copy link

thiakil commented Apr 26, 2019

For the 3 cited, flipping them to match the prefixing pattern works for me. MatcherBlock PatternBlock

@liach
Copy link

liach commented Apr 26, 2019

BlockMatcher and BlockPattern are already suffixed. So this issue is really changing partial suffixing to total suffixing.

@ConnorJC3
Copy link

ConnorJC3 commented Apr 26, 2019

For the 3 cited, flipping them to match the prefixing pattern works for me. MatcherBlock PatternBlock

Disagree here (regardless of the outcome of this change), it makes the names more confusing:
BlockPattern is clearly a pattern, while PatternBlock sounds like it is a class for a block. Classes where one part of the name is a noun and the other is an adjective describing that noun should go AdjectiveNoun, as that's how adjectives are naturally expected to work in english.

(This is different than classes such as FooItem and FooBlock, because for these both parts of the name are nouns and therefore order is more debatable, which is why we are here with this issue.)

@liach
Copy link

liach commented Apr 26, 2019

For the 3 cited, flipping them to match the prefixing pattern works for me. MatcherBlock PatternBlock

First, MCP is not really all-prefix as of now.

Original words Prefix naming Suffix naming Current name in MCP
diamond block BlockDiamond DiamondBlock BlockDiamond
block matcher MatcherBlock BlockMatcher BlockMatcher

All-suffix naming is definitely better than a mix of prefix and suffix (which is what we have now)

@thiakil
Copy link

thiakil commented Apr 26, 2019

@ConnorJC3 Pattern & Block are still both technically nouns. When taken in context to the rest of the names being BlockFoo it makes sense.

@liach did you miss the point where I'm saying the currently non-matching names should change?

@liach
Copy link

liach commented Apr 26, 2019

@liach did you miss the point where I'm saying the currently non-matching names should change?

If a current name is not prefix e.g. BlockState is not prefixed as it is from phrase "block state" instead of "state block", unlike ItemAppleGold is prefixed as it is from "gold apple item", then we do not change.

Given this fact, we cannot change BlockState in this migration as there is only BlockState that is the correct suffixed version of phrase "block state".

@LexManos
Copy link
Member

LexManos commented Apr 26, 2019

If this goes through, BlockState would not change as it is not a subclass of Block nor State.
It is its own concept.
Same goes for things like ItemStack.

If this doesn't pass, things like all the Biome subclasses would be renamed to be consistent.

BlockMatcher would not change however. As it is not a subclass of Matcher. It is a implementation of the Predicate interface.
We would need to define how we name classes that implement interfaces.

BlockMatcher is a good example in the sense that it would need to change in some way:
First question, do we force children of an interface to be named as that interface?
If yes, then BlockMatcher -> BlockMatchPredicate
Then, do we Prefix or suffix {this vote}?
If pass: BlockMatchPredicate -> PredicateBlockMatch

An argument in favor for going suffix, is cases like this. As well as the main JRE following a pattern that appears to be: ImplementationDetailType
Example: List {Interface} -> ArrayList {A List implemented by an array}

@liach
Copy link

liach commented Apr 26, 2019

But what WOULD change is the example above: BlockMatcher -> MatcherBlock
Because that's a implementation of Matcher that matches Blocks.

Hmm, what... I thought prefix versus suffix is that when you have a phrase "apple boy cat dog", prefix form names a class like DogCatBoyApple while suffix form is AppleBoyCatDog. If this is not the case for the supposed "suffix", I'd support the naming style that calls "block matcher" BlockMatcher, "golden apple item" as GoldenAppleItem, etc. instead.

@LexManos
Copy link
Member

Updated my comment after reviewing the implementation of BlockMatcher. Tho, i don't quite understand your "phrase" example.. there are no phrases here. Or at least yours would not make sense. Perhaps a better example would be class Animal {} and class Human extends Animal {} We're debating if the Human class should be named HumanAnimal or AnimalHuman

@liach
Copy link

liach commented Apr 26, 2019

usually if we have human that is a type of animal, we say "human animal" instead of "animal human", hence HumanAnimal instead of AnimalHuman

@liach
Copy link

liach commented Apr 26, 2019

It's explicitly marked as a note.
image
Moreover, this migration cost isn't as large as you may assume; things break as well even when the simple name of class stays but the package changes.
Given the scale the game mechanism has changed is between 1.12 (few mods are on 1.13) and 1.14, the name migration here should be too much of a burden.

@LexManos
Copy link
Member

@liach I just marked it as a note, to clear up any confusion in the future.

@DarkGuardsman
Copy link

Yes a script will solve most of this, that was not a complaint but a note of a solution for the problem. As I already to do this for imports, and enum changes plus interfaces if we also do that. Unlikely to actually take 30 seconds as the build scripts can take 15-30 mins on my workstation. That is if it is not made more complex by more changes and needs to be rerun for failures. However, this is not the problem i'm concerned about.

The real concern is if the mapper doesn't work for 100% of cases and the need to update documentation. On top of the normal length update process and need to rewrite large chunks of mods. Which will be made more complex as we developers have to learn to inverse our current usage of class names. You don't exactly unlearn to use prefixes overnight. Which we will have to unlearn as mixing usage in the same system gets confusing fast. EX: BlockColoredChest extends ChestBlock.

I'm also concerned that once we adopt a standard it will be enforced outside of forge. I've already went through this before with RF API. It became the unofficial standard, all those that didn't use it were constantly asked why they didn't implement or exclusively use it. I still get asked to this day why I have my own internal power API in my mods. Even though I use Forge Energy as my core provider and the API only exists to wrapper IC2 & BC.

Add to this we devs already get into arguments over naming of classes, fields, usage of "I" for interfaces, what to include and not include in API, what to make public vs not to make public, how to format code, what data types to use, etc. You say we can do whatever we want it doesn't work that easily. Once a standard is picked it will slowly ripple out into the community.

It will become what developers expect of each other. Especially when asking each other for help. Someone will provide some code with a simple question "How do I fix insertion slots". Right in the middle of someone else answer another developer will go "Why are you still using prefixes, you know that is legacy?". Then the conversation will devolve and the original developer will not get help. I've seen this happen in several discords and IRC chats before. Its not a unique problem even to the MC modding community. As I see it at work a lot as well, but it is a problem we need to think about when making standardizations.

@dshadowwolf
Copy link

Rather simple question here: This is about changing something that has been a "standing convention" in MCP for a rather long time - yes, it is somewhat counter to what a lot of Java programmers expect and yes, it "makes it more difficult for English speakers" (seriously?) but...

It's currently working and working just fine - why "fix" something that isn't, actually, "broken" to begin with ?

@LexManos
Copy link
Member

LexManos commented Apr 26, 2019

The real concern is if the mapper doesn't work for 100% of cases

Srg2Source has been proven to work on all projects AFAIK. And there are other solutions as well. So this isnt a valid concern.

I'm also concerned that once we adopt a standard it will be enforced outside of forge. ... Then the conversation will devolve and the original developer will not get help

Again, Stop being Forge into this. It has nothing to do with Forge, this is MCP. And if you're complaing that others yell at you about your code style. Do like every other project does in existence and tell them to sod off it's your code. If you don't have the spine to do that in your own projects then there is nothing I can do to help you. Code style/api design/interops/etc.. Will always be a holy war. It's just how it is. We're not going to let that stop us from defining a project standard.

It's currently working and working just fine - why "fix" something that isn't, actually, "broken" to begin with ?

Sounds like a vote to keep the old system, so cast your vote. The 'status quo' argument is a good one. And people should take it into consideration when making their vote.

This is all about actually defining what was previously undefined.
A current example that I just ran into is the Sound classes.

We have: AbstractSound which has two sub classes MovingSound and SimpleSound
SimpleSound has no subclasses, but MovingSound does:
ElytraSound, GuardianSound, SubSound, and UnderwaterSound
But it ALSO has MovingSoundMinecart and MovingSoundMinecartRiding

The entire point is to decide which format to use, and unify things like this. And that's why it is important to define something instead of the nothing we have now.

Again, I am not advocating one side or the other, I am mainly trying to dissuade the "Don't do anything because any change is bad" argument. As there will be change no matter the outcome.

@DarkGuardsman
Copy link

Again, Stop being Forge into this. It has nothing to do with Forge, this is MCP

True, this is about MCP and not forge. I neglect to keep the two separated as both are used together. As a change in MCP is a change in the forge ecosystem.

And if you're complaing that others yell at you about your code style...

I would not have survived in modding if I couldn't deal with people being mean. What I have issues with is watching other developers attack each other rather than working together to solve problems.

Something like this will cause more of that. Especially if tutorials, documentation, and general understanding are not carried forward at the same time. So if we make a massive change we need to mitigate the problem, not say we will after. This needs to be done with documentation, update tutorials, ready to go scripts with clear guides on usage, etc. I'm willing to spend time helping with this if I'm not doing it alone.

Also if we are going to make a massive change can we just overhaul the entire thing? I've found things like TileEntity vs Entity. Inventory vs Container vs GUI, and a few other things to be confusing. We might as well consider it now if we need to rewrite tutorials, documentation, and other materials anways.

@kierajreed
Copy link

I like the prefix style because it is nice for sorting, however it is easier to understand what a class is with suffixes.

@sp614x
Copy link

sp614x commented Apr 26, 2019

I am against the mass renaming prefix -> suffix. For me it has no real advantages and it has a lot of disadvantages.

  • It would make updating from 1.13 to 1.14 much more complex and error prone.
  • It would make porting future development to older versions practically impossible.
  • It would make following the code history much harder.

Many mods and players are staying on older editions for various reasons. Because of this porting new development and bugfixes to older versions is important.

A stable "non-optimal" base is better than a "perfect" permanently shifting base.

@kierajreed
Copy link

There are source remappers that automatically change the class names for you...

@tterrag1098
Copy link
Collaborator

@sp614x Fixing the problem of a "permanently shifting base" is exactly the reason behind these polls. The fact that we have no set standard has led to endless churn regarding the naming conventions. Now is the time to settle on one way of doing things and break the status quo, there won't be another chance.

@NekoCaffeine
Copy link

NekoCaffeine commented Apr 27, 2019

Everything you know will turn red underlined. awesome
I hope Srg2Source works well.

@liach
Copy link

liach commented Apr 27, 2019

Everything you know will turn red underlined

Same goes for package changes when BlockPos was moved from net.minecraft.util to net.minecraft.util.math. Consider using find/replace in path in intellij idea.

@toliner
Copy link

toliner commented Apr 27, 2019

It would make porting future development to older versions practically impossible.

From the viewpoint of backport, using Kotlin typealias is a good solution :)

I think that each of prefix and suffix has both good point and bad point and whether to use prefix or suffix depends on your preference and case-by-case.

I suggest the lowest cost way: choose the most used one in 1.13 for each types.
For example: All subclass of Block are prefixed Block and All subclass of AbstractSound are suffixed Sound.

@DarkGuardsman
Copy link

It might suck but having prefix & suffix in the same code base is not that bad. We can pick the preferred for each system and then make that the standard. The reality of this is that most large code bases do exactly this. I work on a project used by a large global retail giant that has well over 200+ apps to manage their clothing distribution alone.

What you will find is that each app and even subsystem inside the app will have its own standards. We do this as sometimes it makes sense to do it one-way verse another. For example, all our controllers will be ControllerName. While our services will be NameService and NameServiceSubtype. Even still our DB accessors might be ServiceNameDaoAccessor and OracleServiceNameDaoAccessor.

@LexManos
Copy link
Member

I understand everyone stating that we can pick and choose based on whatever parts of the system we want.
However, that is not going to happen. It is going to be a global coding style and that's it. The point is to be consistent.

@liach
Copy link

liach commented Apr 27, 2019

It would make porting future development to older versions practically impossible.

Nah, MCP isn't some sort of API layer that is compatible across versions like Bukkit or Sponge. If you really want that compatibility like 1.8 to 1.12 all in one jar, you should look into Bukkit plugins' NMS tools or just grab mappings from the environment on the run.

@Pokechu22
Copy link

That's not the point of that statement -- it makes having one codebase that can target multiple versions, even if it's multiple JARs with slightly configured buildscripts and all of that, basically impossible. Even if different versions are on different branches, it makes it much harder to backport changes manually, since not only do merge conflicts from actual code changes need to be handled, the various renames also need to be handled. That's manageable with small sets of changes, but something this large and frivolous would be much more painful were it to happen...

@DarkGuardsman
Copy link

There are ways to work around that, one of the solutions I'm considering myself is building custom mappings so I can keep my 'I' and prefix naming. Then run a tool before pushing code to the remote git to restore it to normal for other developers to work with. You can do the same thing to help mitigate time spent supporting several versions.

@Trinsdar
Copy link

I feel that existing suffixes should be renamed to prefixes, since it would be a lot less in changes rather then renaming prefixes to suffixes. I also personally prefer the prefix style.

@AlexModGuy
Copy link

its entirely a stylistic preference, but imho i prefer prefixes as they can keep class types separate expecially for those who have things like BlockCheese and ItemCheese becoming CheeseBlock CheeseItem, which could easily be confused for each other in a quick glance if they were in the same package.

@McJty
Copy link

McJty commented Apr 28, 2019

I prefer suffix. If you want to group all blocks together you can always put them in a 'blocks' package

@AlexModGuy
Copy link

While that I usually do that, it can get a bit more abstract beyond simple items and blocks, especially if your mod does a lot with entities. I often have many things like trackers, enums and interfaces in the same package as entities, just like vanilla does. It is aesthetically pleasing and more intuitive that the Interfaces would be before Entities would be before the Enums, rather than jumbled all around.

@bs2609
Copy link
Contributor

bs2609 commented Apr 28, 2019

@Alex-the-666 this is about vanilla MC names, see the issue description:

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.

You can name your things how you like right now - the decision taken here does not change that.

@DarkGuardsman
Copy link

This is entirely about naming and not packages. As we all have different ways to view working with packages. I personally like to define my packages by feature set and content set. So I will almost always end up with a package "com.builtbroken.modname.content.machine" which could include "ItemMachine, BlockMachine, TileEntityMachine, ContainerMachine, InventoryMachine, GuiMachine, BatteryMachine, FluidHandlerMachine, RecipeHandlerMachine".

However, this has zero impact on how we name the MC classes. As MC ships with no packages normally and MCP will build out the packages in type set with all blocks in one package. Only thing that matters with packages is making sure we don't name two classes in the same package the same. Given we should avoid this anyways to make importing not a nightmare.

@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
=====================================
251: Voted in favor.
93: Voted in opposition.
0: Are indifferent
344: Total votes.
2: 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