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

Use the PotionID when creating the DataContainer (Close #1276) #1712

Merged
merged 1 commit into from
Feb 4, 2018
Merged

Use the PotionID when creating the DataContainer (Close #1276) #1712

merged 1 commit into from
Feb 4, 2018

Conversation

ImMorpheus
Copy link
Contributor

@ImMorpheus ImMorpheus commented Jan 14, 2018 •
edited
Loading

When creating a DataContainer from a PotionEffect, we are using its name as the DataQueries.POTION_TYPE. However if you pass the container to the PotionBuilder it uses the DataQueries.POTION_TYPE to search for the PotionEffect in the registry (which requires the PotionEffect id).

Fix #1276

@ImMorpheus ImMorpheus requested a review from gabizou as a code owner January 14, 2018 16:35
@ryantheleach ryantheleach added type: bug Something isn't working system: data status: needs updating hey, origin changed, you need to update version: 1.12 (u) API: 7 (unsupported since May 21st 2021) labels Jan 14, 2018
@ryantheleach ryantheleach added this to the API 8.0 milestone Jan 14, 2018
@ryantheleach
Copy link
Contributor

ryantheleach commented Jan 14, 2018 •
edited
Loading

I've marked this API8/needs updating because it is changing the contract of a DataContainer.

However, my guess is that since it's a bug fix, and it's only changing the serialization behavior, that the deserialization is currently correct so this doesn't round trip correctly?

With the assumption that it's a bug, I think it's safe for Milestone 7.1 to immediately fix the issue for people.

If it's even remotely possible that it's currently useful to someone in it's current state, I suggest bumping the content version number and making a migrator.

I suggest waiting for feedback though, but my suspicion this is safe to move to 7.1 as is.

@parlough
Copy link
Contributor

@ryantheleach I don't see a point in versioning as it is failing to deserialize already. I mean we could fix the current deserializer through some for loop using the name, which is probably the translation id to properly deserialize, but that's a pain, not guaranteed to work in all cases, and of course was failing already.

@parlough parlough added status: input wanted and removed status: needs updating hey, origin changed, you need to update labels Jan 14, 2018
@ryantheleach ryantheleach modified the milestones: API 8.0, API 7.1 Jan 15, 2018
@ryantheleach
Copy link
Contributor

ryantheleach commented Jan 15, 2018 •
edited
Loading

@Meronat good enough for me. This should be ok for merge then, pending review.

@ryantheleach ryantheleach added the status: needs review a code review is needed label Jan 15, 2018
@gabizou
Copy link
Member

gabizou commented Jan 15, 2018

@Meronat @ryantheleach In my opinion, bumping the content version would resolve the issue while providing the upgrader to automatically fix the potion name -> id usage. This will maintain functionality in legacy data input without injecting a "fixer" in the deserializer. That's what I'd prefer to see happen here.

@ryantheleach ryantheleach added status: needs updating hey, origin changed, you need to update and removed status: needs review a code review is needed labels Jan 16, 2018
@ImMorpheus
Copy link
Contributor Author

ImMorpheus commented Jan 29, 2018 •
edited
Loading

Sorry for the delay. I missed the notification. Thanks to @Meronat for helping me out yesterday

@parlough parlough removed the status: needs updating hey, origin changed, you need to update label Jan 29, 2018
@parlough
Copy link
Contributor

parlough commented Jan 29, 2018 •
edited
Loading

~qa

@limbo-app limbo-app added status: needs review a code review is needed status: needs testing does this run, does it solve the issue etc labels Jan 29, 2018
gabizou
gabizou requested changes Jan 30, 2018
View reviewed changes
Copy link
Member

@gabizou gabizou left a comment

Choose a reason for hiding this comment

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

Minor organizational changes.

src/main/java/org/spongepowered/common/effect/potion/PotionEffectContentUpdater.java Outdated

@Override
public int getInputVersion() {
return 1;
Copy link
Member

Choose a reason for hiding this comment

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

Should make this a DataVersions constant so it's more understandable the differences between said versions.

src/main/java/org/spongepowered/common/effect/potion/PotionEffectContentUpdater.java Outdated

@Override
public int getOutputVersion() {
return 2;
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Should likely be DataVersions.Potions.CURRENT_VERSION

src/main/java/org/spongepowered/common/effect/potion/SpongePotionBuilder.java Outdated
@@ -48,7 +48,7 @@
private boolean showParticles;

public SpongePotionBuilder() {
super(PotionEffect.class, 1);
super(PotionEffect.class, 2);
Copy link
Member

Choose a reason for hiding this comment

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

And reference said constants.

@Zidane
Copy link
Member

Zidane commented Jan 31, 2018

@gabizou

Give me a final word on this one.

dualspiral
dualspiral reviewed Jan 31, 2018
View reviewed changes
src/main/java/org/spongepowered/common/effect/potion/PotionEffectContentUpdater.java Outdated

@Override
public int getOutputVersion() {
return DataVersions.Potion.CURRENT_VERSION;
Copy link
Contributor

@dualspiral dualspiral Jan 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

If we later create a version 3, won’t this break any really old data being migrated, as the migrator would then mislabel version 2 data as version 3?

This should just be version 2 or its own constant, separate from the CURRENT_VERSION constant to avoid this mistake later.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, we can have two pointers, one being the constant 2 for using the proper id, and the "current version" that points to version 2.

@gabizou gabizou added the status: ready to merge This PR is ready to be merged label Feb 2, 2018
@gabizou gabizou removed status: needs review a code review is needed status: needs testing does this run, does it solve the issue etc labels Feb 2, 2018
@gabizou
Copy link
Member

gabizou commented Feb 2, 2018

@Zidane ready to merge. Looks good.

@Zidane
Copy link
Member

Zidane commented Feb 2, 2018

@ImMorpheus

Squash to one commit and I can quickly merge this. Thanks!

@ImMorpheus
Copy link
Contributor Author

Done.

@parlough parlough merged commit f3c4ad5 into SpongePowered:bleeding Feb 4, 2018
parlough pushed a commit to parlough/SpongeCommon that referenced this pull request Feb 4, 2018
@ImMorpheus @parlough
Use the PotionID when creating the DataContainer. (SpongePowered#1712)
067bfb5 
Uses a DataContentUpdater to update the outdated data containers
(Closes SpongePowered#1276)
@ImMorpheus ImMorpheus deleted the fix/issue-1276 branch March 8, 2019 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dualspiral dualspiral dualspiral left review comments

@gabizou gabizou gabizou approved these changes

Assignees
No one assigned
Labels
status: ready to merge This PR is ready to be merged system: data type: bug Something isn't working version: 1.12 (u) API: 7 (unsupported since May 21st 2021)
Projects
None yet
Milestone
API 7.1
Development

Successfully merging this pull request may close these issues.
7 participants
@ImMorpheus @ryantheleach @parlough @gabizou @Zidane @dualspiral @limbo-app