Fixes Issue #64

[Deathwing777]

Uses a simple boolean setting in default_settings.lua to decide which digiline rules to use in common.lua

[Deathwing777]

At first I was planning on exposing it to the formspec, but then I realized that the rules were being applied at a more global level, so changing the behavior of individual node instances wasn't going to be easy. So I opted for a simple server wide configuration option to either use the working digilines rules or the broken pipeworks digiline rules (because builds on the server use the broken behavior as a feature).

[eshattow]

I cannot make sense of what is written in the code comments because it is written as a matter of opinion and diverts away from the code changes and the outcomes. Please remove your opinion of if something is "broken" or "old broken" or "new broken" ... really does not matter and is confusing for some admin trying to understand what outcome the option will have. Describe the outcomes and let the admin have their own thoughts about it when selecting the option. Reasons for an option remaining a default being that it was like this before, good, but you must be specific about what version or release the changes happened (or at least a calendar date).

Edit: specific suggestion example is you say "break connectivity" which has no specific meaning. Broken connectivity may mean that it does connect when it should not, or it does not connect when it should. That is bad documentation. Use instead language that describes what happens as "This option ignores vertical connections above and below the node" "This option enables"... 🤷

[Deathwing777]

The code comments are directed at developers not server admins. It consists of a rationalization for keeping an old rule around in the codebase, and allowing server admins to revert to that behavior.

There are different comments, more like what you describe, aimed at server admins in the settings_type.txt file.

I linked to the github issue in the code comments you are referring to. It has all of the detailed information about edge cases that might want the broken rules instead of the rules that don't break expected behavior of nodes. The github issue also connects to the pull request and to the (eventual) inclusion in the repository, so I don't really understand why you would want to put version information or timestamp in the code comments.

I was using "break connectivity" from a digiline (hardware) perspective. As in the signal does not conduct because the circuit is broken/open (as opposed to short/closed). I can see how you misinterpreted it using "broken" in the more general sense of "it does not work". In any case, reading the linked github issue would make the context clear.

I would have an equally hard time understanding what you mean by "ignores vertical connections" Either it is connected or it isn't. Are you doing some filtering to get it to ignore just some messages?

So maybe something like: "When using these rules, digiline signals are NOT conducted vertically, despite any rotation to the node. This breaks expected behavior with digiline conducting tubes. However, some servers may have builds that relied on the lack of vertical connection and may wish to revert to this behavior."

[wsor4035]

agree with author that comments are for devs, the ones in settings file are for users

anyways, seems a few luacheck issues to resolve

[Deathwing777]

Trailing whitespace in a comment is a luacheck issue apparently.

[eshattow]

So maybe something like: "When using these rules, digiline signals are NOT conducted vertically, despite any rotation to the node. This breaks expected behavior with digiline conducting tubes. However, some servers may have builds that relied on the lack of vertical connection and may wish to revert to this behavior."

Yes, please, and thank you. (Edit: my commentary would be that I know how to read words so 'not' and 'NOT' the all-caps style is distracting I do not need to be shouted at in comments for such a simple statement; also 'revert' is maybe not a clear word choice here, I might suggest 'retain' or 'keep' and really talking about "some servers" is a matter of opinion anyways or is speculative - just drop that last sentence).

[OgelGames]

I think it might be better for the setting to be named "enable vertical digilines connectivity" or similar, at least for me it's less confusing that way.

[Deathwing777]

Are we ready to merge the pull request?

-Edit: Never mind, apparently I needed a page refresh.

[wsor4035]

seems legit

[Deathwing777]

Anyone else have any feedback? Are we ready to merge?

[eshattow]

I've made my review inline at 0e37040

[OgelGames]

I don't know why @eshattow's comment was marked as spam, but this is the review comment, which I agree with:

minor nit, this is fine but could be better said as pipeworks_enable_digiline_connection_vertical to fit the existing pipeworks configuration variable style

Also, the description for the setting needs to be reworded to match the name change.

[S-S-X]

If changing those then could also maybe change setting description a bit, I feel it is unnecessarily verbose and probably could be more about facts and less about recommendations. Anyway, it does explain what setting does so this description rewording would be rather low prio stuff.

[eshattow]

If changing those then could also maybe change setting description a bit, I feel it is unnecessarily verbose and probably could be more about facts and less about recommendations. Anyway, it does explain what setting does so this description rewording would be rather low prio stuff.

I do mention this in the review. Simply the editorial and opinion is, after the factual description, two more lines of text that are not needed and may be deleted.

No idea why or why not things are marked as spam.