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

Revert compatibility break with NodePath and ConfigurationTransformation #105

Merged
merged 1 commit into from
Jun 26, 2018

Conversation

lucko
Copy link
Contributor

@lucko lucko commented Jun 26, 2018

Reverts some breakages introduced in #92

@@ -37,6 +37,6 @@
* @return A modified path, or null if the path is to stay the same
*/
@Nullable
Object[] visitPath(@NonNull NodePath inputPath, @NonNull ConfigurationNode valueAtPath);
Object[] visitPath(ConfigurationTransformation.@NonNull NodePath inputPath, @NonNull ConfigurationNode valueAtPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

@NonNull ConfigurationTransformation.NodePath

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not valid:

The only other way to do it is to add an import statement for

import ninja.leaping.configurate.transformation.ConfigurationTransformation.NodePath

and then have

Object[] visitPath(@NonNull NodePath inputPath, @NonNull ConfigurationNode valueAtPath);

but I thought that was unclear, as an interface with the same name already exists in the same package.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not valid? Huh... Java surprises me every day.

Copy link
Contributor

@dualspiral dualspiral Jun 26, 2018

Choose a reason for hiding this comment

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

To comment: the signature of this method will still have changed slightly (it was SimpleConfigurationTransformation.NodePath), but it actually points to the same class as NodePath's outer class is ConfigurationTransformation, which is the superclass of SimpleConfigurationTransformation. The nuance was likely what caused the confusion with it not being accessible in the previous version - it looked inaccessible from the code that was written, but it actually wasn't.

I checked the bytecode from Nucleus which was compiled using configurate 3.2, it references ConfigurationTransformation.NodePath not SimpleConfigurationTransformation.NodePath - so this change should be fine.

Copy link
Contributor

@kashike kashike Jun 26, 2018

Choose a reason for hiding this comment

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

ConfigurationTransformation.@NonNull NodePath

@dualspiral
Copy link
Contributor

dualspiral commented Jun 26, 2018

Any stylistic comments aside, I think this looks fine, given the comment I gave before.

@kashike It's probably in our interest to get this wrapped up and pushed out of the door sooner rather than later. I'll let you pull the lever if you're happy with this, as I said in my comment, the bytecode in Nucleus references
ninja.leaping.configurate.transformation.ConfigurationTransformation$NodePath, so this should be OK and not an API break.

@kashike kashike self-assigned this Jun 26, 2018
@kashike kashike merged commit 5f4b2f1 into master Jun 26, 2018
@lucko lucko deleted the fix/compatibility branch July 10, 2018 04:08
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.

3 participants