-
Notifications
You must be signed in to change notification settings - Fork 378
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
PacketByteBuf: Rename IntArray to VarIntArray #3186
base: 1.19.1
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, a method reading/writing X
should always be called readX
or writeX
. The var-int part is an implementation detail.
Yes, but imho this is a special case. Minecraft can write ints and var ints. If minecraft only used var ints, then it would be obvious and an implmentation detail. We need to make a clear difference between those two, they might break stuff while reversing the protocol/protocol changes. |
@Bixilon From the user (usually modder, since Fabric is mostly a modding project) perspective, the method's purpose is more important than what it does internally. Even if the method sent the int list by stringifying and concatenating with a space it should still be |
So, I am mainly using yarn to check the (minecraft) protocol changes between 2 versions. When I see What about Normally the name should be what the method is used to or what it does. I'd name your example probably |
@Bixilon writeInt/writeVarInt is named that way only because there are two different methods doing two different things and Java doesn't allow such kinds of overloading (obviously). This time there is only one method. Unless you're checking the raw packet, the information on whether the integer is var or not is useless - and if you're using Yarn for that purpose, javadoc is your friend. |
🚀 Target branch has been updated to 1.19-pre2 |
🚀 Target branch has been updated to 1.19-pre3 |
@apple502j: var ints are not simply implementation details. If there's a list of ints like uuid, it's preferable to be written as regular int array given its uniform distribution. var ints are only good if the value is potentially small; if it's uniformly distributed within the range of int/long, regular int or long are more preferable. |
🚀 Target branch has been updated to 1.19-pre4 |
Can we get this before 1.19? (tomorrow) (then it really might be impactful) |
@Bixilon Generally, impactful renames should not be done during pre-release/release candidate phase. That said, if others agree we should merge this now, then I suppose it's fine. |
This isn't really impactful as this is clarifying and there is no fixed-size-int arrays in |
🚀 Target branch has been updated to 1.19 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO the encoding of the length would be an implementation detail that shouldn't affect the name, but the encoding of each element is very functionally relevant and thus should be reflected in the method name.
so...anything here? |
🚀 Target branch has been updated to 1.19.1-pre5 |
🚀 Target branch has been updated to 1.19.1 |
The name
IntArray
is quite confusing, I am thinking at 4 bytes, not 1-5 bytes.