mt-mods / moretrees

Other
8 stars 10 forks source link

Remove unused optional dep to avoid circular dep #21

Closed Awkanimus closed 1 year ago

Awkanimus commented 1 year ago

Removing ethereal from the optional_depends because it is not used in moretrees and it creates a circular dependency.

The dependency cycle this solves is currently ethereal --> bakedclay --> flowerpot --> moretrees --> ethereal.

Niklp09 commented 1 year ago

https://github.com/mt-mods/moretrees/pull/11#discussion_r1002973356

Was this fixed by TenPlus1?

wsor4035 commented 1 year ago

#11 (comment)

Was this fixed by TenPlus1?

nope

wsor4035 commented 1 year ago

to clarify the label, tenplus1 is to blame here for still nuking things unless mods dep on ethereal to load after it

Niklp09 commented 1 year ago

relevant yl issue https://gitea.your-land.de/your-land/bugtracker/issues/3013

wsor4035 commented 1 year ago

probably should make an issue from tracking the status so we dont forget ^ ill do this

wsor4035 commented 1 year ago

see https://github.com/mt-mods/moretrees/issues/22

Awkanimus commented 1 year ago

Forgot to comment. Thanks for the clarification. I'll watch the related bug.

Awkanimus commented 1 year ago

Updated as requested. Notes:

  1. Made generic so the same mod_dependency_checks.lua could be used in other projects
  2. Dependencies can be marked as "fatal" in which case server does not start when check fails
  3. Dependencies can be marked as "optional" which allows the dependency to be missing
  4. logs at the "error" level are visible to players, all others are not. A summary is displayed to the player (who may be less technical) and an explanation is dropped in debug.txt.
  5. I did try to do this with sending chat messages instead of logs and it didn't work so well; I think the players dont exist while the server is loading or something (which kinda makes sense). Given the desire to halt a server from starting up, I moved on.

edit: Reworded point 3 for clarity

Awkanimus commented 1 year ago

Testing notes:

  1. Tested world with and without ethereal loaded and ensured world loaded.
  2. Tested in world with recent ethereal loaded and ensured world loaded.
  3. Changed the required version by adding "99" to the front and validated that error message was provided
  4. Changed optional to false and revalidated point 3
  5. Changed optional to false and loaded world without ethereal and ensured error (even though this case should have used mod.conf)
Awkanimus commented 1 year ago

For clarity, coding and testing is all done. Not sure if requesting a rereview is the way forward here but that's what I did

wsor4035 commented 1 year ago

im sorry, but these latest commits are absolutely over the top ridiculous. nothing more was needed besides a minimally modified version of the code listed in https://github.com/mt-mods/moretrees/issues/22#issuecomment-1345102761 - additionally, it seems like luacheck wasnt even used here (see failed ci)

Awkanimus commented 1 year ago

As requested, I force-pushed the version much closer to the snippet in the other issue. Luacheck is now happy about it. Thanks for pointing it out; it's my first time in lua. Hope this helps.

Awkanimus commented 1 year ago

I just noticed you folks use tabs, not spaces. Just a moment.

Awkanimus commented 1 year ago

Ok @wsor4035, a version more similar to the one in the other conversation is force-pushed. Tabs, not spaces are used as thats what the code around it mostly uses. Luacheck (assuming luacheck's config plus the .luacheckrc settings are enough) passes.

wsor4035 commented 1 year ago

oh, just noticed moretress should be moretrees in the error function

Awkanimus commented 1 year ago

Ok. The comment is more descriptive, now uses minetest.global_exists("ethereal") and s/moretress/moretrees/g.

Awkanimus commented 1 year ago

Also, manual tests with and without ethereal and with a bad version number rerun and continue to pass. Luacheck passes.

wsor4035 commented 1 year ago

looks good. approval still stands.

wsor4035 commented 1 year ago

thank you for your contribution and testing @Awkanimus

Awkanimus commented 1 year ago

Happy to help. Thanks for the timely responses and constructive feedback.