fix_plugin_lists misses some fixes

[Utumno]

The stack overflow I reported: wrye-bash/wrye-bash@b79a4c2

is still present in your latest iteration: ddb53c7

Reason:

Calling the method again in the except block is asking for trouble anyway

EDIT: second SO:

In this case it is a misordering of mods and masters - especially in this case the mod was inactive

EDIT: edited the title to reflect the liblo rather than the Bash issue. Real fix should be to add to get_** methods an output parameter which would return the fixed list - the list being fixed while being checked in checkvalid() rather than repeating the ifs in a different method. Moreover the invalid lists are of interest to the client (warn the user, make decisions on how to fix etc) - so the model of the library binding should be return valid, invalid and let the user decide (or even return invalid, warning - where invalid should mean actual - more on this later) - rather than 'throw on error then call fix'. Especially the masters test should be dropped from LO as it was dropped from active - it even fails on inactive mods and that's not what the library should worry about anyway - it is the utilities role to display such inconsistencies to the user and let him decide

[Ortham]

The latest master will silently fix invalid load orders just as the game does. Does this resolve your issues?

[Utumno]

Dunno I use mine lol (where I never use fix_load_order)

Will have a look at your latest iteration when I get some time - unifying active and load was needed - don't know how you done it though. A pity you did not take the bump of major version op to drop get/set plugin position - see Utumno#1. Just bump to 8 and drop them now - ask your clients - nobody should be using them. Glad you did drop masters checks. Not sure that silently fixing is good - at least we should be warned.

Two things I would like to see is detailed documentation of validity checks and caching of validity etc checks - I think you did something there (mod times is super fast - the real sink was validity checks ofc, let alone it would just load a couple gigs to memory).

[Ortham]

Just bump to 8 and drop them now - ask your clients - nobody should be using them.

There's no reason why nobody shouldn't be using them. Maybe nobody is, but removing those functions would break the ABI, and there's no point doing that to remove functionality that nobody may be using anyway.

Two things I would like to see is detailed documentation of validity checks and caching of validity etc checks

I can add documentation about what libespm/libloadorder considers to be a valid plugin. #23

Checking validity can be slow for large plugins as it reads the whole file, but that only happens once per plugin when it is added to the load order. I can probably speed it up, maybe just check for a valid header, and I may be able to improve performance by re-using some plugin data when reloading the load order. #24

[Ortham]

Not sure that silently fixing is good - at least we should be warned.

Libloadorder only silently fixes what the game does, so there's no real need for clients to know something is wonky. If they set the load order, then the fixed load order will be saved. If they just get the load order, then they'll get the same load order that the game uses.

[Utumno]

There's no reason why nobody shouldn't be using them. Maybe nobody is, but removing those functions would break the ABI, and there's no point doing that to remove functionality that nobody may be using anyway.

There is - Utumno#1. In all, better drop them now nobody is using them. And of course if nobody is using them it means something - it means they should not be there - and removing them is needed cause someone may start using them (the horror, the horror - cause then the API will break).

You did see that vid by Josh Bloch didn't you ?

If they set the load order, then the fixed load order will be saved.

Leaving the developer to wonder why a different order is set from the one he/she specified for whatever reason - for instance what if you have a bug there (see current issue) ?

Silently should appear nowhere in a lib's docs.

---

Re: API : https://github.com/Utumno/libloadorder/issues?utf8=%E2%9C%93&q=is%3Aissue+label%3AAPI+

Consider your API alpha, and make changes now - and (as explained by Bloch) you must listen to your clients. They 're using it.

[Ortham]

There is - Utumno#1.

There are no reasons there for why a client should not use them. That issue just reasons them to be an unnecessary maintenance burden, because the client doesn't gain anything from using them if you assume their implementation matches yours (which it may not).

and removing them is needed cause someone may start using them (the horror, the horror - cause then the API will break).

There's nothing wrong with using those functions, so no horror or need to later remove them.

You did see that vid by Josh Bloch didn't you ?

Yes, and thank you for the link, it was very educational.

Leaving the developer to wonder why a different order is set from the one he/she specified for whatever reason

You misunderstand, the load order will be set to exactly what the developer sets it to, because they are not allowed to set an invalid load order. It's only when getting the load order that silent fixup occurs, and that fixup is justified because it's exactly what the game does.

Consider your API alpha, and make changes now - and (as explained by Bloch) you must listen to your clients. They 're using it.

I am making changes, see #25, and all the behavioural changes I made in #20. Two of those changes are direct consequences of your feedback.

[Utumno]

There are no reasons there for why a client should not use them.

My point is that it is a conceptual API mistake. I elaborate on this in the issue linked. That 's also a maintenance burden is just a consequence.

As far as I am concerned even set/get single plugin active should be removed - but that is API design level 2. For those here I am sure the library is wrong in allowing the client to set individual order. Activating an individual plugin could make sense (although the library would be much more easy to maintain without and the client lose nothing) - but allowing setting a single plugin's LO is absurd.

Bash only uses 4 methods - get/set Load/active. See also Utumno#5

It's only when getting the load order that silent fixup occurs

Are those fixups in loadorder txt or in plugins's txt ? Are the txts rewritten (they were not leading to cycles) ?
Even in those cases Bash should be notified - like "invalid so-and-so.txt corrected - errors....". Cause I may manually modify the plugins.txt for instance and then wonder why it's not read by Bash - and another couple weeks digging to get to the silent fix.

No silent fixes please.

Will post more feedback when I get round to it

---

Two of those changes are direct consequences of your feedback.

Last but not least - I would like to get write rights here, given the amount of work I put. I find it only fair as you have owner rights in wrye-bash, despite you said you are not interested anymore in working for the project.

[Ortham]

My point is that it is a conceptual API mistake.

I simply diagree, and am willing to agree to disagree on this point.

Activating an individual plugin could make sense

Could? If it didn't make sense, it wouldn't be an operation that could be performed by every mod manager, including the one included in the game launcher.

but allowing setting a single plugin's LO is absurd.

No it's not, if I know that A needs to load before B, and I know B's position, I don't need to know about any positions of any other plugins in the load order to set A's position.

Are those fixups in loadorder txt or in plugins's txt ?

Fixups are in memory, nothing is written when a loaded data is silently fixed. Another reason why fixing silently is safe.

Cause I may manually modify the plugins.txt for instance

Then libloadorder will detect the modification and reload the data from it. Making manual edits while also using libloadorder to handle editing for you is not advised though, because part of the point of libloadorder is to abstract that stuff.

Last but not least - I would like to get write rights here, given the amount of work I put.

I did not request the work you put in, and I disagree with too much of it to give you write access here. Your fork may be perfect for Bash's use case, but you're not the only client, and I think that allowing you to upstream all your changes without review would be an error of judgement on my part.

I find it only fair as you have owner rights in wrye-bash, despite you said you are not interested anymore in working for the project.

If you think this is some sort of quid pro quo, you are mistaken, and I'm perfectly happy to have my membership of the wrye-bash organisation revoked. As you say, I'm no longer interested in contributing to it.

---

It's become clear now that this issue thread has deviated far from its intended purpose, so I'm locking it. If you have another code/design issue you want to discuss, open an issue specific to it. Otherwise, there are more appropriate forums for communication.