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

Add Keys#HANGING for lanterns #2438

Merged
merged 2 commits into from
Aug 16, 2022
Merged

Conversation

Lignium
Copy link
Contributor

@Lignium Lignium commented Jul 30, 2022

SpongeAPI | Sponge

@Zidane
Copy link
Member

Zidane commented Aug 16, 2022

@Faithcaio

Going to merge this for api-8 just to allow this version to have this feature. When you merge this upstream, you'll need to omit this commit. Or deprecate the data entry and mention to use the state properties?

@Zidane Zidane merged commit bdd66ef into SpongePowered:api-8 Aug 16, 2022
@Lignium
Copy link
Contributor Author

Lignium commented Aug 17, 2022

@Zidane If I understand you correctly, why are you suggesting to omit this in API 9? Isn't the Data API the preferred way to work with data? The State Property API is, and has always been, a secondary (fallback) way to get data when the data is not reflected in the Data API. Let me remind you that the more data is implemented using the Data API, the better methods such as mergeWith, values, etc. will work.

You also forgot to merge the implementation.

@Zidane
Copy link
Member

Zidane commented Aug 17, 2022

@Lignium

I didn't forget to merge it, just haven't had the chance to merge your impl PR and bump the API ref. If you wanna help me out further, have your impl PR bump the API ref then it is a clean pull.

Also I only pointed it out to @Faithcaio in-case there was some reason this cannot remain in the data system. If no such reason exists, this will be a clean merge upstream.

@Faithcaio
Copy link
Contributor

Faithcaio commented Aug 17, 2022

Its more work to have the properties as Data atm. as the properties get generated automatically.
Also Keys becomes quite cluttered.

So the best option would be to improve how to register Properties as Data and then deprecate the properties entirely.

Faithcaio added a commit that referenced this pull request Aug 17, 2022
@Lignium Lignium deleted the feature/lantern-data branch August 20, 2022 05:37
Faithcaio added a commit that referenced this pull request Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants