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

complete some things, simple type names, misc #643

Merged
merged 21 commits into from
Oct 3, 2024

Conversation

supersaiyansubtlety
Copy link
Contributor

@supersaiyansubtlety supersaiyansubtlety commented Sep 24, 2024

  • add several simple_type_field_names
  • remove RegistryKey from simple_type_field_names (they're often named after their generic type)
  • map all RegistryKeys affected by its removal from simple_type_field_names (no lost coverage! [except in unmapped classes])
  • complete n.m.village
  • complete n.m.registry
  • complete Items (thanks type names for 1000+ settings)
  • rename/fix some things in IdListUtil
  • make users of IdListUtil::createByIdGetter consistent
  • miscellaneous mappings mapped along the way

@supersaiyansubtlety supersaiyansubtlety added t: new adds new mappings v: snapshot targets a snapshot version of minecraft reviews needed please review this PR s: medium PRs with less than 700 lines and more than 200 labels Sep 24, 2024
@supersaiyansubtlety supersaiyansubtlety self-assigned this Sep 24, 2024
@supersaiyansubtlety supersaiyansubtlety removed the reviews needed please review this PR label Sep 24, 2024
simple_type_field_names.json5 Outdated Show resolved Hide resolved
simple_type_field_names.json5 Outdated Show resolved Hide resolved
simple_type_field_names.json5 Outdated Show resolved Hide resolved
simple_type_field_names.json5 Outdated Show resolved Hide resolved
Co-authored-by: Eli Orona <eliorona@live.com>
…ames

rename IdListUtil#sortArray -> createByIdGetter
make enums that use createByIdGetter consistent
restore SpawnReason
add simple type names for Entity+LivinEntity
add stub FindInvalidSimpleTypeFieldNamesTask class
map a few other things along the way
@supersaiyansubtlety supersaiyansubtlety added s: large PRs with more than 700 lines and removed s: medium PRs with less than 700 lines and more than 200 labels Sep 25, 2024
CLASS net/minecraft/unmapped/C_uxzryfxv net/minecraft/server/function/FunctionGetter
FIELD f_cxzaybvc commandFunctionSet Z
FIELD f_kkregdwp commandFunction Ljava/util/Optional;
METHOD m_ashjquub getid ()Lnet/minecraft/unmapped/C_ncpywfca;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

spell check fail?

@supersaiyansubtlety supersaiyansubtlety changed the title map n.m village and registry complete some things, simple type names, misc Sep 25, 2024
@supersaiyansubtlety supersaiyansubtlety added the reviews needed please review this PR label Sep 25, 2024
@supersaiyansubtlety supersaiyansubtlety marked this pull request as ready for review September 25, 2024 12:36
build.gradle Outdated
Comment on lines 120 to 126
final enigmaProfile = file("enigma_profile.json")

inputs.file(
new JsonSlurper().parse(enigmaProfile)
.services.jar_indexer.args.simple_type_field_names_path
)

Copy link
Member

Choose a reason for hiding this comment

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

This... Works for now. I would make an issue once this is merged to do this properly.

Copy link
Contributor Author

@supersaiyansubtlety supersaiyansubtlety Sep 26, 2024

Choose a reason for hiding this comment

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

afaict a proper fix would require enigma plugins to have a way of listing files they depend on, then enigma would list all files all plugins depend on
hadn't found getServiceProfiles

Is that what you mean?
Or something in qmap buildSrc?

Copy link
Member

Choose a reason for hiding this comment

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

something in the buildsrc

@supersaiyansubtlety supersaiyansubtlety mentioned this pull request Sep 28, 2024
14 tasks
Copy link
Member

@ix0rai ix0rai left a comment

Choose a reason for hiding this comment

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

looks fantastic, just a couple things:

  • there are quite a few empty mappings here, i think dropInvalidMappings should address that
  • though the enum refactors are great, I don't agree with the BY_ID -> GET_BY_ID refactor. i don't see any good reason to add the extra word.

@supersaiyansubtlety
Copy link
Contributor Author

though the enum refactors are great, I don't agree with the BY_ID -> GET_BY_ID refactor. i don't see any good reason to add the extra word.

Yeah I'm a bit torn on byId and GET_BY_ID:

  • strictly by convention they should both have "get"
  • but I prefer the brevity without "get" and I don't think clarity is lost
  • I usually like to name fields holding functional interface objects extra function-y because they're slightly counter-intuitive to me, hence the "get" on the field
  • on the other hand if "by id" is function-y enough for the method it should be function-y enough for the field
  • they also should definitely match

I guess "by id" is fine for both.

@ix0rai ix0rai added final-comment-period is approved and will soon be merged if no issues are raised update-base used to notify github actions that the base branch should be updated and removed reviews needed please review this PR labels Oct 1, 2024
Copy link
Contributor

github-actions bot commented Oct 1, 2024

🚀 Target branch has been updated to 24w39a

@github-actions github-actions bot changed the base branch from 24w38a to 24w39a October 1, 2024 15:08
Copy link
Contributor

github-actions bot commented Oct 1, 2024

🚨 Please fix merge conflicts before this can be merged

@github-actions github-actions bot added outdated this pull request hasn't been updated to the latest version or has merge conflicts and removed update-base used to notify github actions that the base branch should be updated labels Oct 1, 2024
@supersaiyansubtlety supersaiyansubtlety removed the outdated this pull request hasn't been updated to the latest version or has merge conflicts label Oct 2, 2024
@ix0rai ix0rai merged commit 5f11ce6 into QuiltMC:24w39a Oct 3, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period is approved and will soon be merged if no issues are raised s: large PRs with more than 700 lines t: new adds new mappings v: snapshot targets a snapshot version of minecraft
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants