ModCoderPack / MCPBot-Issues

Issue tracker for MCPBot (https://bitbucket.org/ProfMobius/mcpbot) and MCP mappings.
http://mcpbot.bspk.rs
80 stars 11 forks source link

Migrate from prefixes to suffixes #817

Closed kashike closed 5 years ago

kashike commented 5 years ago

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

Please react with :+1: if you are in favour, and :-1: 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 commented 5 years ago

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 commented 5 years ago

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

LexManos commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

LexManos commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

ghost commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

liach commented 5 years ago

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

ConnorJC3 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

LexManos commented 5 years ago

And thus the argument for Suffixes.

Darkhax commented 5 years ago

I personally prefer prefixes. To me it makes more sense that you would name data related things like data rather than treating it like part of a sentence. With that said, the thing I care about above all else is that the class names are consistent with themselves. We currently have a mix of several naming schemes and that is really annoying.

DarkGuardsman commented 5 years ago

A mix is not bad so long as its constant within the system its being used. If this really is about a few classes being named strangely then lets solve that. Rather than making this an XYZ problem and causing a lot of rework.

As well the 'phrasing of' idea only works in english. I often find myself thinking in spanish when coding. Where something is "Object of description of description2 of description3". So making a massive change based on this is not a very good reason.

We need to spend a lot more time talking over the advantage of the change in total. Rather then focusing down on 2-3 examples. Since if we rename all Items and blocks to suffix. This will force every mod updating to 1.14 to run through a mapper. Which is not a quick task for a change that may not improve quality of development.

shift02 commented 5 years ago

It is a question !

Does that apply to anything other than 'Item' and 'Block' ?

Gui, Container, TileEntity etc...

If it is 'TileEntityFurnace' , will it be changed to 'FurnaceTileEntity' ?

toliner commented 5 years ago

I disagree with this change.

The difference between prefix and suffix is only whether you prefer to use prefix or suffix. Most people will not change their mind because prefix vs suffix seems to vim vs emacs.

So, Let's think about migration cost. I think the migration cost is very very high because all mods and documents should be migrated. Not only forge contributors, but also all modders will be forced to pay this cost.

I don't want to pay such a foolish cost.

HellFirePvP commented 5 years ago

I also have to disagree with this change.

I do not really think one is better than the other for significant reasons and don't care how a given mod implements those naming conventions on their end. For one i've seen both prefix- and suffixbased namings in mods and i'm generally fine with anything as long as it's consistent and organized.

Which brings me to the point others have been making in here aswell: Things are constantly changing in modded minecraft. Methods in vanilla classes being renamed, classes having different names eventhough they still have 90% of the same behavior and other things. So documentation across multiple versions is basically worthless to begin with and unless the site in question explicitly states which version their snippet or tutorial is for, things are bound to most likely not work straight up or require further reading of someone who's most likely not very familiar with the codebase's concepts, utilities, existing functionality (which might already do what the developer in question might want to achieve) or related.

So making this general change across the entire codebase across a multitude of classes for the sake of "but we're doing it here too" or "but some names collide and might cause confusion" is in my opinion not a strong enough reason for the amount of time investement into changing everything over to something else, both on forge's as well as the developers' side.

"We did this originally because we couldn't repackage stuff to make it more organized" is not a great reason either. As it stands, it still works out fine and across the entire time we did have repackaging of those classes, i didn't really feel it was a specific problem or hurdle. So if you wanted to back that argument, the logical point is, because we're now able to package it into a 'block' package, we should rename BlockBed, BlockWool, ... to Bed, Wool, ... instead, because "it's in the right package now, so you can see what it is by its package name already.

"mods who don't use packages and follow vanilla's class naming" - in all honesty? I am strongly in favor of splitting at least items and blocks generally into their own packages. Mods that pack items, blocks, biomes and other classes into the same package lack organization in their codebase in my opinion.

All in all, disagreeing with this change.

LexManos commented 5 years ago

Does that apply to anything other than 'Item' and 'Block' ?

Yes, this is defining the global code style for the entire vanilla codebase. So TileEntities, Guis, Containers, Inventories, Biomes, Potions, Enchantments, Packets, etc.. We are doing this to define a standard to follow.

As well the 'phrasing of' idea only works in english

This is true, however, MCP is a English project {as is most of public projects} These standards however, have zero effect on your classes. So you can name yourself whatever you want.

This will force every mod updating to 1.14 to run through a mapper. Which is not a quick task

It's a 1 line command, with about 30 seconds to run. This is not a valid argument against in my opinion. As the BULK of the update time will be in all the OTHER code changes that have happened.

I think the migration cost is very very high because all mods and documents should be migrated. Not only forge contributors, but also all modders will be forced to pay this cost.

Yes, documentation is a major cost of this change. there is no autmatic way to migrate those. And that's unfortunate, and needs to be taken into account. However, this has nothing to do with Forge. As this is a MCP change that I am leaving completely up to the community. So please do not bring Forge into this.

i'm generally fine with anything as long as it's consistent and organized.

That's the issue right now, Vanilla code base isn't consistent. This is an attempt to make it.

"We did this originally because we couldn't repackage stuff to make it more organized" is not a great reason either

I provided that information in the other thread purely as a historical note, not a argument for or against.

liach commented 5 years ago

Just curious, if we stay on the old prefix, would existing suffix names like AdvancementCommand be turned into prefix? It has been a suffix name since its introduction in 1.12. #456

LexManos commented 5 years ago

Yes they would. Again, this is defining the global code standard and the entire code base will be refactored to be consistent. So there is a cost either way.

HellFirePvP commented 5 years ago

I provided that information in the other thread purely as a historical note, not a argument for or against.

Pardon me, i saw it listed as reason for the change on the original post up above, so i responded to it as if it was one of the arguments for the change. Sorry, i didn't read all answers and their individual interpretation intentions on this thread and the other and all threads linked to this. Will do so next time to see if the mentioned reasons are the reasons put forth by the one posting it or not.

liach commented 5 years ago

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 commented 5 years ago

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

DarkGuardsman commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

sp614x commented 5 years ago

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

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 commented 5 years ago

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

tterrag1098 commented 5 years ago

@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 commented 5 years ago

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

liach commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.