A small amount of 1.20.2 mappings #479
Conversation
EnnuiL
added
t: new
mappings/net/minecraft/item/SuspiciousStewEffectContainer.mapping
Outdated
Show resolved
Hide resolved
Co-authored-by: Will <[email protected]>
|
|
Co-authored-by: ix0rai <[email protected]>
|
@EnnuiL do you intend to fix this up, or should I take it over? |
|
oh god i completely forgot about this pr; i have been quite busy recently, but i'll see if i can tackle it later |
|
actually, please take it over |
OroArmor
added
the
update-base
|
🚀 Target branch has been updated to 23w35a |
github-actions
bot
removed
the
update-base
Co-authored-by: Will <[email protected]>
ix0rai
added
the
final-comment-period
| @@ -36,6 +36,8 @@ CLASS net/minecraft/unmapped/C_wwadquuj net/minecraft/block/entity/BeaconBlockEn | |||
| ARG 1 x | |||
| ARG 2 y | |||
| ARG 3 z | |||
| METHOD m_jlfysuud getEffect (Lnet/minecraft/unmapped/C_jaqasomh;)Lnet/minecraft/unmapped/C_jaqasomh; | |||
There was a problem hiding this comment.
i still have a bad feeling about getting rid of the orNull; it just feels reasonable here, it's like, a standard too;
please reconsider it before merge
There was a problem hiding this comment.
Can you give an example of a similar method with the OrNull suffix?
I've been comparing it to Map#get, but there could be a better comparison.
There was a problem hiding this comment.
Actually maybe part of the issue is get might be the wrong verb here.
It checks if the beacon has the effect, returns it if so, or null otherwise.
It's like a weird contains method that returns arg/null instead of true/false.
There was a problem hiding this comment.
This method seems to be replacing what mojmap called getValidEffectById, I think valid is important.
I also think effect in the name is redundant given the parameter.
How about just validate?
There was a problem hiding this comment.
after looking through the source, I agree with @supersaiyansubtlety on validate being the best name
I'm going to add javadoc too
There was a problem hiding this comment.
@EnnuiL please take a look at the new name!
There was a problem hiding this comment.
Looks good to me; I'll double-check on Enigma soon, since uh, getContainerOrNull still lost the OrNull and i remember that it was there for good reasons
There was a problem hiding this comment.
it's annotated as @Nullable which imo makes the suffix redundant always
There was a problem hiding this comment.
it's annotated as @Nullable which imo makes the suffix redundant always
No description provided.