Lightning enchantment

[FordFriedel]

Adds "lightning" enchantment to swords, which strikes lightning and deals additional damage to entities when the player is below the sky during a storm.

[oddlama]

Generally your code looks great! There are some minor formatting issues, but that's mostly me and my "religious beliefs" regarding that matter. Nothing an auto formatter wouldn't fix anyways.

Now that I see your enchantment I must say that I do like it. Although it arguably doesn't fit into the vane spirit regarding strictly useful additions (and there are already those trident thingies that have a similar feature). As the enchantment is really low-maintenance, I'd be willing to include your changes under the compromise that would be disabled by default. All the players who use vane for the "vanilla enhancements" stuff wouldn't be put off by a fun addition.

EDIT: If I'm not mistaken, you can add this function to your enchantment to disable it by default:

public boolean config_enabled_def() {
    return false;
}

[FordFriedel]

So by writing the code:

public boolean config_enabled_def() { return false; }

this will automatically write to the config file that our feature should be disabled by default? Or is further editing needed to achieve this.

[oddlama]

Yep, but it won't change existing configurations (so you might want to just delete yours to test it. It will be regenerated.)

You might have noticed that vane has it's own configuration system which makes it so easy to add configuration options anywhere in the code. That specific function here is a part of this special configuration system. It isn't an overridden function but behaves similar to one. If this function exists, the system will simply use it to determine the default enabled status for the submodule (the enchantment).

[oddlama]

Looks good so far! Next step would be testing that everything works as expected, and then it's finished I guess :)

[FordFriedel]

One problem before in game testing: the enchantment is enabled by default, do I need to return true to disable the enchantment by default?

[FordFriedel]

after a quick test its clear that I will want to protect the user from getting struck by their own lightning, maybe I take it one step further and make anyone yielding a lightning sword immune to taking damage from lightning

[oddlama]

One problem before in game testing: the enchantment is enabled by default, do I need to return true to disable the enchantment by default?

No, not really. The function returns the default state for the enabled config field, so true would make it enabled by default. Did you regenerate the configuration by deleting vane-enchantments/config.yml to test it? Maybe I didn't remember the name correctly. I can double-check later.

after a quick test its clear that I will want to protect the user from getting struck by their own lightning, maybe I take it one step further and make anyone yielding a lightning sword immune to taking damage from lightning

Sound like a good thing. Could also be made configurable (see @ConfigBoolean) :)

[FordFriedel]

Should be ready for re-review and formatting requests

[oddlama]

Looks very very good! Please see the comment I wrote, there's just one little thing that needs fixing because I told you something incorrectly (sorry). After that we can merge :)

[FordFriedel]

Sorry I am just a little confused. I understand the idea of modules in vane to allow for users to customize their experience by toggling features on and off.

What I dont understand is do you want me to return true? I believe I had that previously and that turned it on by default.

[oddlama]

What I want you to do is to remove the function config_enabled_def completely and instead add this constructor:

public Lightning(Context<Enchantments> context) {
    // Don't enable this enchantment by default
    super(context, false);
}

[FordFriedel]

Okay, this change prompts an error because the Lightning constructor on my version does not anticipate a boolean to be passed.

[oddlama]

Yes, that's why I stated you need to rebase on the newest commit on the develop branch. I just added this parameter yesterday for you :)

[oddlama]

If you don't want the hassle with rebasing and switching to 1.19 i can also do this myself if need be. It's important that you rebase and not merge to preserve a clean history.

[FordFriedel]

After a stressful bit I have rebased. I believe that it's ready to go. Thanks.

[oddlama]

Thanks for your hard work! Sorry for causing you to go through the rebasing trouble, but I guess this also is an essential part of writing PRs :P. I'll become second nature.

I also agree that this looks ready to merge now. I'm a bit busy at the moment, but will merge this when I find time. I'll also release a new minor version as soon as paper 1.19 gets out of experimental, just in case I have to fix some other things which might pop up over the next days.

[FordFriedel]

Sounds good! I am trying to accrue more topics to my repertoire so I appreciate having to struggle through new things. Big thank you for giving me a platform to learn through!