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: don't analyze WML within [filter_wml] blocks (most checks are not applicable to its contents) #3790

Open
wants to merge 2 commits into
base: master
from

Conversation

@edwardspec
Copy link
Contributor

commented Dec 18, 2018

This fixes situation when wmllint tried to add description=_"fireball"
to the following WML code:

[filter]
    [filter_wml]
        [not]
            [attack]
                name=fireball
            [/attack]
        [/not]
    [/filter_wml]
[/filter]

Tags within [filter_wml] don't need to have all mandatory arguments
(like "description"), because:

  1. they are temporary constructs and this description is never used,
  2. such WML code expects that results will be filtered ONLY by name.
    Adding description will remove results that have the same attack name
    but different description.
wmllint: don't add description to [attack] tags within [filter_wml]
This fixes situation when wmllint tried to add description=_"fireball"
to the following WML code:

[filter]
    [filter_wml]
        [not]
            [attack]
                name=fireball
            [/attack]
        [/not]
    [/filter_wml]
[/filter]

Tags within [filter_wml] don't need to have all mandatory arguments
(like "description"), because:
1) they are temporary constructs and this description is never used,
2) such WML code expects that results will be filtered ONLY by name.
Adding description will remove results that have the same attack name
but different description.
@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

Probably should just skip everything within a [filter_wml] tag (not sure if this commit does that or not).

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

(That said, I'd recommend using [has_attack] instead of [filter_wml] here.)

@edwardspec

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2018

I added early escaping of [filter_wml] blocks.
Any checks will treat them as comments. (they are unescaped after the checks)

I'd recommend using [has_attack] instead of [filter_wml] here

Thanks.

In the future, we should probably add wmllint checks for [attack] or [status] within [filter_wml].
The function hide_filter_wml() is a convenient place to do it.

edwardspec added a commit to edwardspec/Legend_of_the_Invincibles that referenced this pull request Dec 19, 2018

Replace [filter_wml] with more efficient filters (status, has_attack)
It was noticed in wesnoth/wesnoth#3790
that these [filter_wml] are superfluous.

@edwardspec edwardspec changed the title wmllint: don't add description to [attack] tags within [filter_wml] wmllint: don't analyze WML within [filter_wml] blocks (most checks are not applicable to its contents) Dec 19, 2018

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2018

Probably should just skip everything within a [filter_wml] tag

on the other hand if for example a key in unit is renamed, lets assume the situation that we for some reason renamed the attribute image_icon to just icon we would want to update the image_icon occurances in filter_wml aswellso that these filter still work.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Dec 20, 2018

While you do have a point, I'm not sure we can reasonably assume that the contents of [filter_wml] will always be a unit.

Unrelated: @Elvish-Hunter should presumably take a look at this PR.

@sevu sevu requested a review from Elvish-Hunter Apr 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.