"unknown speakers" and other wmllint matters

[Groggy_Dice]

I know that shadowm is not the only one concerned about some of my recent commits, so I'd like to explain them.

on new magic comments being "undocumented":

In fact, I added text to wmllint's introduction explaining how "who" and "whofield" work, which is the same way its magic comments have been documented in the past (except "no-syntax-rewrite", if you're looking for undocumented mcs). I have thought about creating a MagicComments page on the wiki for the wmltools mcs, but haven't found the time.

on the "unknown speaker" errors:

Yes, I only recently realized that wmllint wasn't checking the speaker= key along with id=, and added this check. Yes, like the id check, it tends to have an irritating number of false positives. However, the new check has already turned up a broken last breath message in South Guard. A modest result, but the code in mainline has been picked over for years by experienced developers. There would surely be more true positives in UMC.

As for errors in specific campaigns:

SotBE:

This one is on me, and I've already fixed it. As I explained in the commit message, my working branch had gotten out-of-sync with the master branch, and predated mattsc's wave of changes to the campaign. In particular, mattsc added a side field to the orcish shaman macros (so the shamans could start the Barag Gor scenario on side 3), and I neglected to take into account that this meant their id field had shifted one to the right.

DW:

Only 'Siddry' in one scenario is still tripping the error message, and I'd appreciate it if someone else inserted the "wmllint: recognize" comment. I haven't gotten that far in the campaign, and I'd like to avoid spoilers. I did go ahead and take care of the loyal characters, and sure enough, I caught a peek at the nature and number of the captives in Slavers.

UtBS:

For Rogrimir/Jarl/Grog/Nog, see my full commit message.

That leaves "speaking_unit" in S11. The reason this speaker fails is that this is defined by a variable, not by role=, so "speaking_unit" is not in the "present" list. Like the id check, the speaker check skips values that start with a '$', so if this instance had used the value "$speaking_unit.id" like the other instances in UtBS, it wouldn't have errored. (My testing also turned up the "Spiritual Advisor" problem in S7b, but that had already been found and fixed in upstream master.)

TRoW:

Since my commit messages apparently are going unread, perhaps I should point out the El'Isomithir "issue", and whether it's worth fixing or not.

on not being on IRC:

I am still reading the logs, but I am busy with real-life (or at least non-Wesnoth... lol) projects, and will be for at least a few more months. At one point, I even fell a month behind on the logs. The threatened feature freeze (which I found out from the logs doesn't apply to wmltools - good to know) did prod me to action, but I have to prioritize, and return Wesnoth to the back-burner. As I've noted before, I hadn't been a "chat" fan prior to this, and I don't have time to hang out in IRC right now. But I'm still lurking, still reading the logs (don't know what Napster is?!), and if a bug turns up in my work, I'll try to clean up my own mess.

[Iris]

I know that shadowm is not the only one concerned about some of my recent commits, so I'd like to explain them.

[...]

on the "unknown speaker" errors:

Yes, I only recently realized that wmllint wasn't checking the speaker= key along with id=, and added this check. Yes, like the id check, it tends to have an irritating number of false positives. However, the new check has already turned up a broken last breath message in South Guard. A modest result, but the code in mainline has been picked over for years by experienced developers. There would surely be more true positives in UMC.

Upon closer inspection of my campaign after I finally had time following the release, pretty much all of the unknown speaker warnings I was getting there were caused by me forgetting to use “wmllint: recognize” directives (I can’t use “wmllint: who <macro> is <character>” because wmllint traverses the directory tree in alphanumerical order) in several places and by sheer coincidence not triggering unknown id warnings, so it’s not really a problem. So, apologies for panicking.

As for mainline, the mainline campaigns really aren’t my field and I’d rather stay away from several of them — hopefully their respective maintainers can do something about the warnings later. For me it’s just enough to have wmllint run clean on data/core since that’s the only path that’s relevant for us UMC maintainers, and that is already covered.

[Elvish_Hunter]

(I can’t use “wmllint: who <macro> is <character>” because wmllint traverses the directory tree in alphanumerical order)

After having tested it, I think that a better solution may be to implement a # wmllint: globally recognize <unit_id> (or campaign recognize, or directory recognize. Your call). For example, let's assume that Galas makes wmllint print a lot of unrecognized speaker messages. Instead of placing

Code: Select all

# wmllint: recognize Galas

in every scenario, you could just place in the first one, or even in the _main.cfg

Code: Select all

# wmllint: globally recognize Galas

to completely shut wmllint's warnings about him.
By the way, I can't use wmllint: who either, because I create some units using macros that require additional arguments. For example,

Code: Select all

#define ELYSSA X Y TYPE

is enough to forbid me from using

Code: Select all

# wmllint: who ELYSSA is Elyssa

Groggy, what's your opinion on this proposal?

[Anonymissimus]

# wmllint: globally recognize <unit_id> (or campaign recognize, or directory recognize. Your call).

There already are
# wmllint: local spelling(s)
# wmllint: directory spelling(s)
# wmllint: general spelling(s)
so I suggest using at least the same wording, just in case. (And ideally the same code.)

[Groggy_Dice]

Upon closer inspection of my campaign after I finally had time following the release, pretty much all of the unknown speaker warnings I was getting there were caused by me forgetting to use “wmllint: recognize” directives

I believe your campaigns use RECALL_POS for some recalls. If you converted to the new core macro RECALL_XY, those ids would be automatically picked up. You would have to keep a RECALL_XY define for previous Wesnoth versions, which would cause wmlscope to complain about duplicated resources, but it might save you from having to do a few ": recognize: comments.

By the way, I can't use wmllint: who either, because I create some units using macros that require additional arguments. For example,

Code: Select all

#define ELYSSA X Y TYPE

is enough to forbid me from using

Code: Select all

# wmllint: who ELYSSA is Elyssa

Actually, "who" only cares about the macro name, not whether it has any arguments. So you could use it!

After having tested it, I think that a better solution may be to implement a # wmllint: globally recognize <unit_id> (or campaign recognize, or directory recognize. Your call). For example, let's assume that Galas makes wmllint print a lot of unrecognized speaker messages. Instead of placing

Code: Select all

# wmllint: recognize Galas

in every scenario, you could just place in the first one, or even in the _main.cfg

Code: Select all

# wmllint: globally recognize Galas

to completely shut wmllint's warnings about him.
Groggy, what's your opinion on this proposal?

Well, truth be told, I'm not a fan. The purpose of the id check is to make sure that the specified unit was actually created/recalled. Suppose in one scenario, you forgot to insert the macro, or mistyped it. Then the id warnings would alert you. But if the character was automatically recognized in every scenario, you would get no warning.

(This in fact is why, in the case of UtBS, I didn't suggest tying "wmllint: who" to the MESSAGE_ACCORDING_TO_ALLY macro. That would cause the four possible ally characters to be recognized, but since that macro doesn't actually recall the characters, it wouldn't alert the developer if they forgot to actually recall the ally. So it feels a little sloppy to me.)

Also, between the auto-recognition of the core unit creation/recall macros, and the "who/whofield" magic comments, far fewer characters should be chronically tripping "unknown id" warnings these days. To me, this seems like a solution to a problem that is mostly in the past.

[Iris]

I believe your campaigns use RECALL_POS for some recalls. If you converted to the new core macro RECALL_XY, those ids would be automatically picked up. You would have to keep a RECALL_XY define for previous Wesnoth versions, which would cause wmlscope to complain about duplicated resources, but it might save you from having to do a few ": recognize: comments.

I appreciate the advice, but I actually use a series of custom macros for a number of reasons, and (for AtS, at least) they are defined in files that wmllint won’t see before reading scenario WML. For example, CHARACTER_STATS_GALAS is defined in macros/character-defines.cfg, which wmllint will read after episode1/scenarios/01_The_Skirmish.cfg because, in lieu of true WML preprocessor directive handling, it can only traverse the filesystem in alphabetical order, and macros/ goes after episode1/.

(Having a finished campaign and being able to copy-paste directives all over the place as required (since there is a lot of wmllint functionality that requires me to do this anyway), I don’t see a reason to rename macros/ to _wmllint_please_read_this_first/ or such.)

Also, I don’t use wmlscope.

[Groggy_Dice]

Well, now Siddry in Dead Water won't need a "wmllint: recognize" magic comment. I just added a capability to wmllint for recognizing unstored units.

I appreciate the advice, but I actually use a series of custom macros for a number of reasons, and (for AtS, at least) they are defined in files that wmllint won’t see before reading scenario WML. For example, CHARACTER_STATS_GALAS is defined in macros/character-defines.cfg, which wmllint will read after episode1/scenarios/01_The_Skirmish.cfg because, in lieu of true WML preprocessor directive handling, it can only traverse the filesystem in alphabetical order, and macros/ goes after episode1/.

I'm not sure you have a correct understanding of the way who/whofield is meant to work. You can insert them into scenarios, not at the macro defines. The alphanumerical sorting problem comes into play for those who haven't numbered their scenarios (e.g., instead of 01_Surprise_Attack.cfg, 02_Fleeing.cfg..., you have Surprise_Attack.cfg, Fleeing.cfg...).

[AI]

That would require placing the who/whofield directive in every scenario, or in an unrelated the file that wmllint happens to read first. The former doesn't differ from the recognize directive, while the latter has its own issues.