Skip to content

Conversation

@Lignium
Copy link
Contributor

@Lignium Lignium commented Jul 30, 2022

SpongeAPI | Sponge

@Faithcaio
Copy link
Contributor

@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