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

Adds Entity -> InventoryHolder Converter #5821

Closed
wants to merge 1 commit into from
Closed

Adds Entity -> InventoryHolder Converter #5821

wants to merge 1 commit into from

Conversation

NotSoDelayed
Copy link
Contributor

Description

This PR adds a converter Entity -> InventoryHolder, which fixes #5755 and any potential related undiscovered and future issues.

Target Minecraft Versions: any
Requirements: -
Related Issues: #5755

@AyhamAl-Ali AyhamAl-Ali added bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. enhancement Feature request, an issue about something that could be improved, or a PR improving something. labels Jul 12, 2023
@TheLimeGlass TheLimeGlass added the needs testing Needs testing to determine current status or issue validity, or for WIP feature pulls. label Jul 13, 2023
@TheLimeGlass TheLimeGlass self-requested a review July 13, 2023 06:29
@APickledWalrus APickledWalrus self-requested a review July 14, 2023 12:09
@Pikachu920
Copy link
Member

maybe I'm totally off base with this but doesn't this fix a symptom rather than the root cause? in the linked issue (#5755), the issue is that inventory of attacker is null. the attacker in that issue is a Player, and Player implements InventoryHolder. If this PR fixes the issue by adding a Entity -> InventoryHolder converter, wouldn't that mean that somewhere in Skript something is failing to recognize that a Player IS an InventoryHolder? skript should probably be able to handle this without an explicit converter if so.

@TheLimeGlass
Copy link
Collaborator

TheLimeGlass commented Sep 11, 2023 •
edited
Loading

Adding converters is very sensitive and can break everything.
Converters allows Skript to convert to other things, and things that it shouldn't be converting to, which introduces new issues.
They're typically a last ditch effort or only added for newly added types.

@TheLimeGlass TheLimeGlass added up for debate When the decision is yet to be debated on the issue in question and removed enhancement Feature request, an issue about something that could be improved, or a PR improving something. labels Sep 11, 2023
@TheLimeGlass TheLimeGlass changed the base branch from master to dev/feature October 5, 2023 23:14
@TheLimeGlass TheLimeGlass added enhancement Feature request, an issue about something that could be improved, or a PR improving something. and removed bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. labels Oct 5, 2023
@TheLimeGlass TheLimeGlass removed their request for review October 5, 2023 23:15
@sovdeeth
Copy link
Member

sovdeeth commented Nov 3, 2023

Closing in favor of #6171

@sovdeeth sovdeeth closed this Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@APickledWalrus APickledWalrus APickledWalrus approved these changes

Assignees
No one assigned
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something. needs testing Needs testing to determine current status or issue validity, or for WIP feature pulls. up for debate When the decision is yet to be debated on the issue in question
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot get ANY inventory in Death Event
6 participants
@NotSoDelayed @Pikachu920 @TheLimeGlass @sovdeeth @APickledWalrus @AyhamAl-Ali