Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wmllint: "speaks in die event" may read the id from the wrong filter #4085

Closed
stevecotton opened this issue May 19, 2019 · 5 comments

Comments

Projects
None yet
4 participants
@stevecotton
Copy link
Contributor

commented May 19, 2019

The following code example triggers a false-positive warning that Ras-Tabahn speaks in his "die" event rather than "last breath". Seen with the wmllint from 1.14.7, it seems wmllint assumes that the first [filter] tag with an id= is the event's filter.

    [event]
        name=die
        [filter]
            type="DoE Mortuary Caravan"
        [/filter]
        [lift_fog]
            [filter]
                id=Ras-Tabahn
            [/filter]
        [/lift_fog]
        [message]
            id=Ras-Tabahn
            message= _ "You left the caravans unprotected? I expected you to at least hide them in the castle."
        [/message]
        [endlevel]
            result=defeat
        [/endlevel]
    [/event]
@jostephd

This comment has been minimized.

Copy link
Member

commented May 19, 2019

I assume the error is this one:

wesnoth/data/tools/wmllint

Lines 1453 to 1493 in d513c0d

# Detect units that speak in their death events
filter_subject = None
die_event = False
deathcheck = True
for nav in WmllintIterator(lines, filename):
if "wmllint: deathcheck off" in nav.text:
deathcheck = False
continue
elif "wmllint: deathcheck on" in nav.text:
deathcheck = True
if "[/event]" in nav.text:
filter_subject = None
die_event = False
elif not nav.ancestors():
continue
elif "[event]" in nav.ancestors():
parent = nav.ancestors()[-1]
if parent == "[event]":
# Check if it's a death event
fields = parse_attribute(nav.text)
if fields:
(key, prefix, value, comment) = fields
if key == 'name' and value == 'die':
die_event = True
elif die_event and not filter_subject and parent == "[filter]":
# Check to see if it has a filter subject
if "id" in nav.text:
try:
(key,prefix,value,comment) = parse_attribute(nav.text)
filter_subject = value
except TypeError:
pass
elif die_event and filter_subject and parent == "[message]":
# Who is speaking?
fields = parse_attribute(nav.text)
if fields:
(key, prefix, value, comment) = fields
if key in ("id", "speaker"):
if deathcheck and ((value == filter_subject) or (value == "unit")):
print('"%s", line %d: %s speaks in his/her "die" event rather than "last breath"' \
% (filename, nav.lineno+1, value))

I guess the error is on line 1468: it shouldn't recurse upward past the [lift_fog] tag.

@stevecotton

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

Line 1463 looks as if it wouldn't handle nested events.

stevecotton added a commit to stevecotton/wesnoth that referenced this issue May 20, 2019

wmllint: Handle nested events and other things with filters in the de…
…ath check (closes wesnoth#4085)

The main reason for moving this to a separate function was to make the
per-event variables local to that function.

stevecotton added a commit to stevecotton/wesnoth that referenced this issue May 27, 2019

wmllint: Handle nested events and other things with filters in the de…
…ath check (closes wesnoth#4085)

The main reason for moving this to a separate function was to make the
per-event variables local to that function.

soliton- added a commit that referenced this issue Jun 3, 2019

Merge pull request #4089 from stevecotton/wmllint_nested_events
wmllint: Handle nested events and other filters in the death check (closes #4085)
@stevecotton

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

@soliton- thanks, but please would you merge to master too?

@soliton-

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

Good point, I‘m rarely in a proper dev environment though so it’d help if you can make PR for that as well.

@stevecotton

This comment has been minimized.

Copy link
Contributor Author

commented Jun 9, 2019

It'll probably be another week before I have time to make the PR for master. Possibly quicker, because it mainly needs the computer's time for building and running the tool, and less of my time for sanity-checking the results.

@jostephd jostephd added the Fwdport label Jun 9, 2019

@jostephd jostephd added this to the 1.15.0 milestone Jun 9, 2019

stevecotton added a commit to stevecotton/wesnoth that referenced this issue Jun 13, 2019

wmllint: Handle nested events and other things with filters in the de…
…ath check (closes wesnoth#4085)

The main reason for moving this to a separate function was to make the
per-event variables local to that function.

(cherry picked from commit 7e69da7)

stevecotton added a commit to stevecotton/wesnoth that referenced this issue Jun 13, 2019

wmllint: Handle nested events and other things with filters in the de…
…ath check (closes wesnoth#4085)

The main reason for moving this to a separate function was to make the
per-event variables local to that function.

(cherry picked from commit 7e69da7)

soliton- added a commit that referenced this issue Jun 13, 2019

wmllint: Handle nested events and other things with filters in the de…
…ath check (closes #4085)

The main reason for moving this to a separate function was to make the
per-event variables local to that function.

(cherry picked from commit 7e69da7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.