Mai parms tests #6802
Mai parms tests #6802
Conversation
There was a problem hiding this comment.
Looks good. I think my main complaints would be:
warningis new in Lua 5.4, and I think this is a great place to use it.- We probably need a deprecation message and support for MicroAIs that have not been updated to the new style, since add-ons may contain their own custom MicroAIs.
| if (number ~= math.floor(number)) then | ||
| return false | ||
| elseif (math.type(number) ~= 'integer') then | ||
| std_print("Warning: [micro_ai] tag (" .. ai_type .. ") parameter '" .. name .. "' must be an integer. It has an integer representation, but is provided in floating-point format.") |
There was a problem hiding this comment.
I think it might make sense to use warning instead of std_print here? I believe that adds it to the Lua console.
| is_wrong_type = not is_number(k_value, false, ai_type, k_name) | ||
| elseif (k_type == 'integer') then | ||
| is_wrong_type = not is_number(k_value, true, ai_type, k_name) | ||
| elseif (k_type == 'csv_integer') then |
There was a problem hiding this comment.
-
I'm not entirely sold on the name here. I wonder if "int_list" or "integer_list" would be easier to understand?
-
Maybe this is a low priority, but is there any demand for a list of real numbers?
| for k_name,k_type in pairs(required_keys) do | ||
| if (k_type == 'tag') then | ||
| -- Check that this is not a scalar parameter | ||
| if cfg[k_name] then |
There was a problem hiding this comment.
This is probably fine in the vast majority of cases, but keep in mind that it's possible to have a tag and a key with the same name. I think in that case it makes more sense to issue a warning rather than an error, if that's not too much work.
I guess it's not a priority though.
| v = v:sub(2,-2) | ||
| for child in wml.child_range(cfg, v) do | ||
| table.insert(CA_cfg, T[v](child)) | ||
| for k_name,k_type in pairs(optional_keys) do |
There was a problem hiding this comment.
Okay, sooo… I think this system (both loops) possibly counts as an API, which means this change should go through a deprecation cycle? What this means is essentially that you'd:
- Add an
if type(k_name) = 'number'case which uses the original code (wherek_typeis actually the key andk_nameis ignored). - Add a one-time deprecation message in this case. Since this isn't a place where
deprecate_apiis possible, you'd probably do something like (pseudo-code)if not showed_message then showed_message = true show_message() end. Theshowed_messagevariable would be a file-level local variable.
|
@CelticMinstrel Replying to all you comments in one message: I did not like The stack trace of Lua's new warning function is nice, but I think that there needs to be a message in the log also, not just in the Lua console. So right now I have both, with the log message pointing to the Lua console for more information (although the stack trace just points to the I did not add anything about there being a key and a tag with the same name right now. I think this is so unlikely that anybody actually does that that we do not need to support it. And the current error should be clear enough for people to know what to do about it. Deprecating the old syntax: meh - I guess since it is possible to set up your own Micro AI using the default I've committed all those in separate commits, so that it is easy to see what I did. I'll squash those before merging. |
| @@ -262,7 +299,8 @@ function micro_ai_helper.micro_ai_setup(cfg, CA_parms, required_keys, optional_k | |||
|
|
|||
| -- Check whether there are invalid keys in the [micro_ai] tag | |||
| for k in pairs(cfg) do | |||
| if (k ~= 'side') and (k ~= 'ai_type') and (k ~= 'action') | |||
| if (not showed_deprecation_message) -- otherwise this produces false positives | |||
There was a problem hiding this comment.
I suspect this check isn't what you want it to be. It's true if there has ever been a deprecation message raised for any Micro AI, not whether there has been a deprecation message for the specific Micro AI being considered.
I'm not quite sure what the best approach is here. You could just skip numeric keys (no wait, you already are? why doesn't that work?), or you could change the deprecation message to be once per Micro AI (so showed_deprecation_message becomes showed_deprecation_message[ai_type]).
There was a problem hiding this comment.
I'm not quite sure what the best approach is here. You could just skip numeric keys (no wait, you already are? why doesn't that work?),
Because this is the loop over the [micro_ai] tag config, not over required_key or optional_keys.
or you could change the deprecation message to be once per Micro AI (so
showed_deprecation_messagebecomesshowed_deprecation_message[ai_type]).
Hmm. I actually figured that getting a deprecation message only once overall was good enough. It gets reset at each reload, after all, and deprecation messages are for UMC authors while they work on their add-ons, not something you need to worry about while you use an add-on.
However, there's a related (bigger?) issue with this, in that if one UMC creator does not bother dealing with deprecation messages, that might disable these warnings for another author. So maybe making it MAI type specific is the way to go.
There was a problem hiding this comment.
Ohh, it shows false positives because the code within this loop doesn't handle the old required/optional keys format? Is that the reasoning?
If that's the case, it feels like it'd be better to put the "if not showed_deprecation_message" outside the loop… though that adds another level of indentation…
There was a problem hiding this comment.
Ohh, it shows false positives because the code within this loop doesn't handle the old required/optional keys format? Is that the reasoning?
Yes.
If that's the case, it feels like it'd be better to put the "if not showed_deprecation_message" outside the loop… though that adds another level of indentation…
Yeah, I know. I did it this way because if I put it around the outside, then that whole chunk of code has to have its indentation changed whenever this is removed in the future, even though it is just two little pieces of conditional that need to be removed and it will look like a much bigger change than it is. And it's not like this is a performance issue. But it is a little "less clean" this way, so I can move it outside if you prefer. It's admittedly not a very good reason to do it the way I did.
There was a problem hiding this comment.
Ah, you have a good point there. Fair enough, we can leave it like this.
It's not a good reason to do it the way you did it, but in my opinion it is a good reason not to bother with the effort of rearranging it now that it's done.
Previously it was added for both the side and the side leader, now it is added only for the side.
|
Okay, sounds good. |
This adds checks for the types of [micro_ai] tag parameters, addressing #6408. It also adds warning messages if a parameter specified in a [micro_ai] tag is not valid for that MAI.
And while I was at it, I also added a couple bug fixes that I came across while working on this.
I am not planning to backport this to 1.16, as the risk of screwing something up in the stable series is too high.