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.

Sign up for GitHub

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

Jump to bottom

Removed 'is' prefix from field/param names as per #987 #1403

Closed
wants to merge 8 commits into from
Closed

Removed 'is' prefix from field/param names as per #987 #1403

wants to merge 8 commits into from

Conversation

dexman545
Copy link
Contributor

@dexman545 dexman545 commented May 29, 2020 •
edited

Kept in some places where it doesn't really fit to remove them:
World's isClient
NativeImage's isSbtImage
PageTurnWidget's isNextPageButton

I believe that's all of them.
In reference to #987

i509VCB
i509VCB suggested changes May 29, 2020
View reviewed changes
Copy link
Contributor

@i509VCB i509VCB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally think the isClient field on the client should become client, but that deserves it's own consideration in a different pr.

mappings/net/minecraft/block/AbstractBlock.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/client/MinecraftClient.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/client/MinecraftClient.mapping Outdated Show resolved Hide resolved
mappings/net/minecraft/entity/passive/HorseBaseEntity.mapping Outdated
@@ -1,7 +1,7 @@
CLASS net/minecraft/class_1496 net/minecraft/entity/passive/HorseBaseEntity
FIELD field_18118 PARENT_HORSE_PREDICATE Lnet/minecraft/class_4051;
FIELD field_6955 temper I
FIELD field_6956 IS_BRED_HORSE Ljava/util/function/Predicate;
FIELD field_6956 BRED_HORSE Ljava/util/function/Predicate;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, what does this predicate do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Used to set the parent predicate, so that child stay around their parent

mappings/net/minecraft/world/gen/feature/ShipwreckFeatureConfig.mapping Outdated Show resolved Hide resolved
@YanisBft
Copy link
Contributor

The issues with this PR are:

  • Non-adjective fields are confusing without "is" prefix
  • Some fields use the "is" prefix because the related json strings use it (damage source and entity predicates)

@YanisBft YanisBft added refactor A PR that renames existing names. snapshot A PR that targets a snapshot version of Minecraft labels May 29, 2020
@dexman545
Copy link
Contributor Author

Restored some old ones

Earthcomputer
Earthcomputer suggested changes May 29, 2020
View reviewed changes
Copy link
Contributor

@Earthcomputer Earthcomputer left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm against some of these changes in cases where the removal of the is prefix causes the field name to be a noun phrase rather than an adjective phrase, which seems to be quite a few of them. Some examples:

  • RealmsWorldSlotButton.State.isCurrentlyActiveSlot -> currentlyActiveSlot: makes it sound like the state "has" a currently active slot, rather than "is" one.
  • AbstractBlock.isAir -> air: makes it sound like an AbstractBlock somehow "has" air, rather than "is" air.
  • You get the pattern.

Some renames that are okay because they turn them into adjectives without losing clarity:

  • BipedEntityModel.isSneaking -> sneaking
  • PlayerEntity.isSubmergedInWater -> submergedInWater (edit: apparently this one is bad because there is already protected Entity.submergedInWater.)
  • etc

@YanisBft YanisBft added the outdated A PR that targets an outdated branch. label May 29, 2020
@dexman545 dexman545 changed the base branch from 20w21a to 20w22a May 29, 2020 23:54
@YanisBft YanisBft removed the outdated A PR that targets an outdated branch. label May 30, 2020
liach
liach reviewed Jun 1, 2020
View reviewed changes
mappings/net/minecraft/client/Mouse.mapping Outdated
@@ -3,7 +3,7 @@ CLASS net/minecraft/class_312 net/minecraft/client/Mouse
FIELD field_1780 activeButton I
FIELD field_1781 controlLeftTicks I
FIELD field_1782 cursorYSmoother Lnet/minecraft/class_3540;
FIELD field_1783 cursorLocked Z
FIELD field_1783 isCursorLocked Z
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No

Suggested change
FIELD field_1783 isCursorLocked Z
FIELD field_1783 cursorLocked Z

@liach
Copy link
Contributor

liach commented Jun 1, 2020

@dexman545 Please remove Closes #987 from pr description. This one does not fix that issue as it misses many usages.

@dexman545 dexman545 changed the base branch from 20w22a to 1.16-pre1 June 4, 2020 21:38
@dexman545 dexman545 changed the base branch from 1.16-pre1 to 1.16-pre2 June 5, 2020 22:50
@YanisBft YanisBft added the outdated A PR that targets an outdated branch. label Jun 8, 2020
@dexman545 dexman545 changed the base branch from 1.16-pre2 to 1.16-pre3 June 10, 2020 17:19
@liach liach added outdated A PR that targets an outdated branch. and removed outdated A PR that targets an outdated branch. labels Jun 11, 2020
@liach
Copy link
Contributor

liach commented Jun 11, 2020

Mind fix the merge conflicts?

@YanisBft YanisBft removed the outdated A PR that targets an outdated branch. label Jun 11, 2020
@liach liach added the outdated A PR that targets an outdated branch. label Jun 11, 2020
@dexman545 dexman545 changed the base branch from 1.16-pre3 to 1.16-pre4 June 11, 2020 21:16
@dexman545 dexman545 marked this pull request as draft June 11, 2020 21:17
@YanisBft YanisBft removed the outdated A PR that targets an outdated branch. label Jun 11, 2020
@liach liach added the outdated A PR that targets an outdated branch. label Jun 12, 2020
@haykam821
Copy link
Contributor

Fixes #987

@dexman545 dexman545 changed the base branch from 1.16-pre4 to 1.16.1 June 25, 2020 09:04
@YanisBft YanisBft removed outdated A PR that targets an outdated branch. snapshot A PR that targets a snapshot version of Minecraft labels Jun 26, 2020
@YanisBft
Copy link
Contributor

This PR is pretty much cursed, and has been badly merged with base branch. If you want to do it again, please make sure to take feedback here into account

@YanisBft YanisBft closed this Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liach liach liach left review comments

@i509VCB i509VCB i509VCB requested changes

@Earthcomputer Earthcomputer Earthcomputer requested changes

Assignees
No one assigned
Labels
refactor A PR that renames existing names.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dexman545 @YanisBft @liach @haykam821 @Earthcomputer @i509VCB