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

API for adding Entity-renaming DataFixes #213

Draft
wants to merge 6 commits into
base: 1.19
Choose a base branch
from

Conversation

xanderstuff
Copy link

I noticed SimpleFixes doesn't have a method for renaming Entities like it does for Items, Blocks, and Biomes. This PR adds that.

@xanderstuff xanderstuff requested a review from a team as a code owner November 6, 2022 04:13
@xanderstuff
Copy link
Author

xanderstuff commented Nov 6, 2022

Note:

  1. This is based off of 1.18.2 Minecraft and pasted into GitHub's web editor because I currently don't have enough space on my PC to setup another project in my IDE. So I wouldn't be surprised if I made 1-2 mistakes.
  2. This has not been tested

@xanderstuff
Copy link
Author

I think these Labels apply:

  • Library: misc
  • s: waiting for test
  • t: new api

@xanderstuff
Copy link
Author

xanderstuff commented Nov 6, 2022

hmm, should a test for this be added to testmodV3 as well?

@Leo40Git Leo40Git added t: new api This adds a new API. s: waiting for test This pull request is waiting to be tested, the PR cannot be put in FCP until it has been tested. library: misc Related to the misc library. labels Nov 6, 2022
@xanderstuff
Copy link
Author

xanderstuff commented Nov 6, 2022

AH! I forgot an import... like I said, I'm working with the webeditor here

@OroArmor OroArmor requested a review from a team November 15, 2022 05:37
Copy link
Contributor

@EnnuiL EnnuiL left a comment

Choose a reason for hiding this comment

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

While we do wanna kill the DFU API later, it doesn't hurt too much to add a few things until it's replacement comes

@xanderstuff
Copy link
Author

just a heads up: I'm currently swamped with work for at least a month. So if testing this is a blocking task, then I won't have time to do it for a while

@xanderstuff
Copy link
Author

While we do wanna kill the DFU API later, it doesn't hurt too much to add a few things until it's replacement comes

is there any existing discussion on this planned replacement I can follow? is it being tracked in a GH issue and/or on the discord server?

@EnnuiL
Copy link
Contributor

EnnuiL commented Dec 4, 2022

While we do wanna kill the DFU API later, it doesn't hurt too much to add a few things until it's replacement comes

is there any existing discussion on this planned replacement I can follow? is it being tracked in a GH issue and/or on the discord server?

Leo has discussed some potential ideas for the replacement on the toolchain server's #forum-qsl forum channel

@EnnuiL
Copy link
Contributor

EnnuiL commented Dec 4, 2022

Anyway, I'm fast-tracking this PR's FCP to Monday, December 5th

@xanderstuff
Copy link
Author

xanderstuff commented Dec 4, 2022

I'd advise against initiating FCP, because it's currently untested and I'd hate to be the author of potentially non-functional code in QSL. I really don't know if it works, I just know it compiles. Doesn't the tag s: waiting for test mean we should wait until it's tested?

@xanderstuff
Copy link
Author

xanderstuff commented Dec 4, 2022

I'm not sure how lenient we are for betas to potentially have bugs, and I suppose that may be a part of an efficient development cycle, but I feel anything involving the DFU needs to have extra care taken because one of the consequences of any bugs/mistakes here includes world corruption (specifically, messing up all entities in the world)

@xanderstuff xanderstuff marked this pull request as draft December 4, 2022 06:52
@EnnuiL
Copy link
Contributor

EnnuiL commented Dec 5, 2022

After some thinking and considering the IRL circumstances, I'm delaying the merging of this PR until I'm able to test it myself; After all, it isn't really as urgent as other merge-before-1.19.3 PRs since DFU API is disabled on 1.19.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period library: misc Related to the misc library. s: waiting for test This pull request is waiting to be tested, the PR cannot be put in FCP until it has been tested. t: new api This adds a new API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants