Module talk:Armor

Compatibility
Summary of the issues encountered: The fixes for 1 and 3 seem fine to me, the workaround for 2 feels rather crude to me. For 5, the solution could be to store the upgrade data for TotK on a separate page (maybe a  page?) but this would need a 2nd crude workaround. Another would be to modify the restriction on StoreAs, perhaps changing it from one table to one table per game, if that's possible? Third would be to rework how the module stores and retrieves data so that BotW data and TotK data are separate in cargo but both handled by this module. However, until the upgrade data is complete or a solution for 4 is found, the optimal solution to 5 seems to be a moot point. 15:31, 2 June 2023 (UTC)
 * 1) module has a game parameter but only actually supports BotW (previous fix)
 * 2) module only allows data from Armor and Armor Upgrade
 * 3) *TotK Armor is stored on Armor in Tears of the Kingdom (previous workaround)
 * 4) while using Armor Data to retrieve upgrade information, the Cargo query doesn't check which game the data is from
 * 5) * would return BotW upgrade data if available (previous fix, same edit as in previous point)
 * 6) Storing to cargo has weird behaviour if Sell Price column has an empty value
 * 7) *Storage of that row fails and any edit, even null edits, cause duplication of data in the Cargo table
 * 8) **Consequently, the previous fixes were removed while identifying this issue
 * 9) Upgrade data for TotK's Armor is not being stored due to the restriction on Data Table's StoreAs parameter, the module's storeUpgrade function automatically sets this as Armor


 * Thanks for looking into this! Here are my first impressions based on the little time I've had to look into this so far.


 * Solutions to #1 and #3 seem fine to me as well.


 * For #2, one solution could be to allow data to be stored on any mainspace (or Data) page, rather than restricting it to specific articles. If there was a reason why I didn't go that route in the first place, I don't remember it. It may have just been an oversight on my part. The main thing is to prevent data from leaking into the Cargo tables via user sandboxes and the like.


 * For #4, I can't think of a root cause off the top of my head. If I had more time, the first thing I would try would be to log the object created on line 217 of the current version (the data being passed to the /Store template via the  parameter of the expandTemplate function) and see if there might be anything wrong with the data being passed to Cargo.
 * I can't recall ever having seen Cargo tables behave this way before. The closest thing I can think of was that one time it refused to store any rows at all. That turned out to be an issue of some data not matching their column type—the template was trying to put strings into an integer column or something like that. It was silently failing the first invalid #cargo_store call and every subsequent one.


 * For #5, I'm not sure I grasp the problem. Is it that the Armor Upgrade page is trying to store two data tables with the same name, and it's not working because storeAs has to be unique per page?
 * I'll try to find more time to look at the code and understand the problem better. PhantomCaleb (talk) 23:36, 7 June 2023 (UTC)
 * For #4, it might be the same underlying issue as the one you described where a non-integer is being put into an integer column and it silently fails when storing that row. The duplication of the data before the failed store wasn't seen in the other case because it failed to store any rows. But I don't know why a blank cell is being read as a non-integer or how to fix that.
 * For #5, yes, this is what I thought was the issue but while writing this reply I realized that I was misinterpreting a number of things and came to a wildly wrong conclusion. Issue #5 didn't actually exist, it was just issue #4 happening at the first row because the first Sell Price was missing. I can confirm this as after this edit, the cargo table now stores the first six rows of the TotK upgrade data and it seems to be failing on the 7th as the defense, another integer, is missing (those six rows are also being duplicated in cargo after editing). 10:30, 8 June 2023 (UTC)