added method ifExists for ease of use

[ghost]

Method takes in a consumer so users can easily check if a node is not virtual and work with it without having to separate things into if statements.

All tests created passed on my end but can be tested again if wanted

[kashike]

Perhaps make this return the same ConfigurationNode, and add an associated ifVirtual?

node
  .ifVirtual(thing -> ....)
  .ifExists(thing -> ...);

I'm also not sold on the ifExists name.

[dualspiral]

Just want to start by saying this is really just an opinion and to not consider this a rejection.

---

I'm not convinced about this in this form because this could very quickly turn into being a mapping function by accident. These lambdas were never intended to be pure functions by any means but they shouldn't be affecting the object that calls them in this instance.

The problem is that, generally, ifPresent(x -> ...); will return the enclosing value rather than the wrapping object, most obvious example here is Optional<T>#ifPresent(Consumer<? super T>) and not allow for the alteration of the parent object. The Optional in question is immutable anyway, but it gives you quick access to the value. Your proposal doesn't do that, it's effectively a glorified if statement and nothing more. Worse, you can alter more that just the value with this ConfigurationNode, while no lambda in Java can be made to be pure, we're just inviting the side effects here and it's not entirely obvious that can happen - hence my comment about being an accidental mapping function.

I would have less of a problem if changes made to the node in the function weren't reflected in the node that called the function. We could do this if we make a ConfigurationNode#copy() before using it in the consumer (so that changes to this node won't affect the main tree), but then we potentially spend a lot of time making this copy to simply bypass an if statement.

I see the problem as to why you chose to return the ConfigurationNode (we don't know the value of the type without asking for it as a TypeToken), but it is what it is.

While I like using functional forms, it doesn't seem right to do so here, particularly if you want to modify the basic node after - if that's the case, you really should stick to the standard if statements. Otherwise, I would advise you looking at making this either a mapping function .mapIfVirtual(node -> return node);, .mapIfPresent(node -> return node); or by considering if some of the getters should have OptionalX or Optional<X> forms, because at least then it would be consistent with functional terms.

---

If this goes ahead as is, the javadocs must be crystal clear as to how this works and, if it remains like this, any mutations to the configuration node inside the lambda will be reflected in the node itself, that it is not a copy.

EDIT: To clarify - I'm not going to lose sleep over this and I'm not stopping this, my point is from a more pure point of view. As long as it's clear what the method does (particularly about the mutation of the node) I'm not going to cry foul.

[ghost]

Perhaps make this return the same ConfigurationNode, and add an associated ifVirtual?

node
  .ifVirtual(thing -> ....)
  .ifExists(thing -> ...);

I'm also not sold on the ifExists name.

Any suggestions as an alternative to ifExists? I asked in the discord and that was the consensus but am open to whatever is most descriptive.

[ghost]

Just want to start by saying this is really just an opinion and to not consider this a rejection.

I'm not convinced about this in this form because this could very quickly turn into being a mapping function by accident. These lambdas were never intended to be pure functions by any means but they shouldn't be affecting the object that calls them in this instance.

The problem is that, generally, ifPresent(x -> ...); will return the enclosing value rather than the wrapping object, most obvious example here is Optional<T>#ifPresent(Consumer<? super T>) and not allow for the alteration of the parent object. The Optional in question is immutable anyway, but it gives you quick access to the value. Your proposal doesn't do that, it's effectively a glorified if statement and nothing more. Worse, you can alter more that just the value with this ConfigurationNode, while no lambda in Java can be made to be pure, we're just inviting the side effects here and it's not entirely obvious that can happen - hence my comment about being an accidental mapping function.

I would have less of a problem if changes made to the node in the function weren't reflected in the node that called the function. We could do this if we make a ConfigurationNode#copy() before using it in the consumer (so that changes to this node won't affect the main tree), but then we potentially spend a lot of time making this copy to simply bypass an if statement.

I see the problem as to why you chose to return the ConfigurationNode (we don't know the value of the type without asking for it as a TypeToken), but it is what it is.

While I like using functional forms, it doesn't seem right to do so here, particularly if you want to modify the basic node after - if that's the case, you really should stick to the standard if statements. Otherwise, I would advise you looking at making this either a mapping function .mapIfVirtual(node -> return node);, .mapIfPresent(node -> return node); or by considering if some of the getters should have OptionalX or Optional<X> forms, because at least then it would be consistent with functional terms.

If this goes ahead as is, the javadocs must be crystal clear as to how this works and, if it remains like this, any mutations to the configuration node inside the lambda will be reflected in the node itself, that it is not a copy.

EDIT: To clarify - I'm not going to lose sleep over this and I'm not stopping this, my point is from a more pure point of view. As long as it's clear what the method does (particularly about the mutation of the node) I'm not going to cry foul.

That brings up a good point. The point to this originally was never to make a copy but actually to make mutating the node easier. I’ll look into some alternatives like your example of mapIfVirtual() but otherwise I will spruce up the javadocs to be clear it isn’t just a copy but the node itself.

[dualspiral]

Honestly, copying isn't really what we want to do here because it's just too expensive. I don't particularly like the idea of the mutating lambdas, but if we name it something else and clearly doc it, I'd settle for that.

To that end, we should go for whenPresent(?) and whenVirtual, or some when prefixed methods. The when differentiates this from Java ifX methods.

Also, while I can understand the idea behind the chaining idea, consider this:

node.whenVirtual(node -> node.setValue(42))
    .whenPresent(node -> System.out.println(node.getValue()));

If you simply implement this in the way your PR does it now, both whenVirtual and whenPresent will fire, because after the whenVirtual call is made, the node is made non-virtual and so whenPresent will fire. So, if we are going down that route you'd need to figure out how to solve that problem - this is part of why I think mutating is a bad idea in this scenario. So, making these void returning methods would be more sensible, I think, because it reduces the possibility of misunderstanding/error in logic.

If you wanted to do that in a functional style, it would be better to simply specify whenPresentOrElse(Consumer<? extends ConfigurationNode> whenPresent, Consumer<? extends ConfigurationNode> whenAbsent); - though at that point, an if statement is a lot more readable and probably more performant (by a tiny margin) - and I suspect we should just go for that in that case.

[ghost]

I’d argue the idea of using (not replacing entirely) if statements with a function ifPresent() or whenPresent() functional method is the next step in the modern java landscape. It would be the same reason why ifPresent() exists for optional. Yes, you can easily just check if it’s present then get the optional from there but on a regular basic I choose the functional route just because it saves me a few lines of code which is quite helpful in the long run.

Take for example the following example:

if(rootNode.getNode("Message").isVirtual()) {
     Text message = Text.of(rootNode.getNode("Message).getValue());
     player.sendMessage(message);
}

That isn't a huge amount of work but that can all be merged into

rootNode.getNode("Message").whenPresent(msg -> player.sendMessage(Text.of(msg));

It isn't a massive change but can be used in a lot of ways and can shorten up hundreds of lines of code if there are lots of isVirtual() checks for highly configurable plugins. In the above code we eliminate the need to have boolean checks in config to see if a message should be sent for example. Now you can simply just not have the line there in the first place and have no second thought to it. Personally, that would be quite helpful for a GUI Shop plugin I'd like to work on as it'd need to have many options for the item in the GUI that are optional. I could use the following when creating the itemstack:

node.getNode("Item#, Name).whenPresent(node -> stack.offer(Keys.DISPLAY_NAME, node.getString());

I could do that for each new offer and cut down the code a bit without sacrificing readability or any extra resources.

In terms of the mutability, if the javadocs clearly defined that it does get the actual node and not just a copy of the node, it shouldn't create problems for anyone. This allows for the programmer to still easily use the lambda function without having to expend resources with another copy() call. Plus this adds in extra functionality if someone did want to mutate the node inside the lambda. That wouldn't be my personal use but it is there if someone wanted it.

I'd say that although the PR is a small change, it can really cut down on unwanted if statements everywhere. The push for more functional workings with Java is great and can be used to simplify a lot without writing extra methods which is why I made this PR in the first place.

[dualspiral]

I get what you're trying to do and why. I never said I didn't. I said I disagree with this solution, but have also provided mitigations and thoughts on alternatives - and that I'm not going to lose sleep over this.

I've made my points that I stand by and I don't really want to let this issue spiral out - especially as I have already said in a previous comment that I will settle for clear documentation and I wasn't going to fight against it.

[XakepSDK]

I disagree with this PR.

1. Configurate is about configuration, not dynamic data storage, use database if you want to store data. Sponge has built-in h2db support.
2. Use configurate auto-mapping system. You shall never manually parse data from ConfigurationNode except serializer/deserializer and/or custom ObjectMapper.

[ghost]

I generally do use h2 DBs for storage and many of my large plugins are using ConfigSerializable for the config anyway. This was just a solution for a much more dynamic plugin that requires quick checks without having to setup a mapper.

However this seems to be pretty contested and I can live without it so I'll take down this PR.