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

Accept [filter] in StandardUnitFilter #4316

Open
ProditorMagnus opened this issue Sep 5, 2019 · 19 comments

Comments

@ProditorMagnus
Copy link
Contributor

commented Sep 5, 2019

Goal is to solve the issue of some tags asking for [filter], and other tags asking for content of [filter].

I think it would make most sense to treat

[filter]
  filter_1
  [filter]
    filter_2
  [/filter]
[/filter]

as

[filter]
  filter_1
  [and]
    filter_2
  [/and]
[/filter]

This has been discussed before, I think there were technical issues in supporting this.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

Goal is to solve the issue of some tags asking for [filter], and other tags asking for content of [filter].

can you please explain this more, i dont really understand whats the issue here.

@AI0867

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

Some tags are a StandardUnitFilter, with a few extra keys. Some contain a [filter] that is a StandardUnitFilter.

Example of the former: https://wiki.wesnoth.org/DirectActionsWML#.5Bkill.5D
Example of the latter: https://wiki.wesnoth.org/DirectActionsWML#.5Bmodify_unit.5D

@GregoryLundberg

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

Would it be possible to maintain a state indicating we've seen a filter clause? In the OP, the first form, with the embedded [filter] should be a syntax error. However, in a [kill] enclosing the SUF in [filter] should only provoke a warning.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

Some tags are a StandardUnitFilter, with a few extra keys. Some contain a [filter] that is a StandardUnitFilter.

Yes i know, but whats the problem with that?

@stevecotton

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

I'm in two minds about the change itself, but if this isn't merged then I think we should print a warning when finding a [filter] inside an SUF.

@GregoryLundberg

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

@gfgtdf The issue as I see it is, unless you know it already, or have the WML manual pages on screen for each tag, it can be surprising that some tags need [filter] and others don't.

The question, as I see it, is how to apply the principle of least surprise ... allowing the WML through when the intent is clear and disallowing obviously wrong constructs.

@jostephd

This comment has been minimized.

Copy link
Member

commented Sep 7, 2019

I support the request, to treat [filter][filter] the same as [filter][and]. It will make it so it's always valid to use an explicit [filter] tag. This will make it easier to write WML code.

I also agree that if we do not make this change, then we should make [kill] [filter] a warning or an error, since what it will do (nothing, [filter] will be ignored) isn't what the WML author intended.

@GregoryLundberg Thumbs down on your suggestions. I see no reason to make [filter] [filter] foo [/filter] [/filter] a syntax error. Maybe it could be a lint warning, but as far as the engine is concerned, it should be valid syntax and have the obvious meaning (same as [filter] foo [/filter]).

@jostephd

This comment has been minimized.

Copy link
Member

commented Sep 7, 2019

Note that [filter][filter_location] and [filter_location][filter] both work already. Making it work when the outer and inner tags have the same name is completely logical... it's redundant, sure, but so are the extra parentheses in ((40 + 2)), and that's valid syntax in pretty much every arithmetic expressions parser I've ever used.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Sep 7, 2019

I would think [filter][filter] is already treated as an error by the schema, right? Though I guess adding it to wmllint might be more helpful, since probably more people use that than the schema.

Keep in mind that [filter][filter_location] and [filter_location][filter] aren't a simple matter of nested filters - they each have a specific meaning. A [filter_location] in a [filter] means you want to match units who are present on a hex matching the nested filter, while a [filter] in a [filter_location] means you want to match hexes that have a unit on them matching the nested filter. In particular, simply including [filter] within [filter_location], even as an empty tag, adds the requirement that there is a unit on that hex.

Also, if we make [filter][filter] equivalent to [filter][and], I wonder if that would lead people to think that [filter_location][filter] is also equivalent to [filter][and]? I think it could especially cause problems in tags that take the contents of a [filter_location] rather than a [filter_location] tag...

@ProditorMagnus

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2019

I have no objections to changing [filter][filter] to error. That is, SUF which finds [filter] reports it as error.
It is half of solution though, then would need something to catch the case of missing [filter] for other tags.

@jostephd

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

@CelticMinstrel Good point about empty tags. Considering that, I think what I would expect instead is that [filter][filter][/filter][/filter] would be equivalent to [filter][/filter], that is, that both of them would mean "there is a unit" (if the parent tag is [filter_location]), or "$unit is not null" (if the parent tag is [event]), etc.

@jostephd

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Goal is to solve the issue of some tags asking for [filter], and other tags asking for content of [filter].

Another direction: How about if we renamed all tags that expect the content of filter? For example, we'd rename [kill] to [kill_filter] and deprecate [kill]. This way the name of the tag will indicate whether it expects a [filter] subtag or SUF keys and tags: a tag that has _filter in its name expects SUF keys and values, other tags are either deprecated or expect a [filter] subtag.

Or maybe we do it differently, deprecate all tags that expect SUF keys and subtags and add new versions of them that expect a [filter] subtag (and error out of it's missing). So, we'd deprecate [kill] and add a [kill2] that must always have a [filter] subtag.

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

I'm not sure if you quite got my point - that [filter] in StandardLocationFilter has a specific meaning, and accepting [filter] in StandardUnitFilter might lead to confusion between tags that accept StandardUnitFilter tags and those that accept StandardLocationFilter tags.

I'm not sure I like renaming all the tags, to be honest. Certainly, [kill2] is a terrible name, but I don't like [kill_filter] that much either.

Is there some list of tags that are relevant to this issue, I wonder? Or two lists, really - those that accept SUF keys and those that accept a [filter] tag with SUF. Perhaps most importantly... how many of them aren't ActionWML?

@jostephd

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

I'm not sure if you quite got my point - that [filter] in StandardLocationFilter has a specific meaning, and accepting [filter] in StandardUnitFilter might lead to confusion between tags that accept StandardUnitFilter tags and those that accept StandardLocationFilter tags.

I did miss your point, then, but I'm still not sure I see what you're getting at... there is no tag that accepts both SUF keys and SLF keys, is there, so it's not like we'll be introducing ambiguity? And making [filter][filter] wouldn't cause the problem you're describing, either; that problem already exists, if a tag accepts SLF keys then that tag can be used as [tag][filter] which looks like an SUF.

I don't have that list handy but I'd like to see it too :)

@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

There is at least one tag (I think it was [goal]) that accepts both SUF and SLF keys, but it's controlled by an additional key, so I'm not sure if that counts.

@stevecotton

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

WML authors are likely to continue using 1.14 for some time, and the warning could be backported to 1.14. I think it would be worthwhile to implement the warning, even if 1.15 then goes on to implement the feature too.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

@gfgtdf The issue as I see it is, unless you know it already, or have the WML manual pages on screen for each tag, it can be surprising that some tags need [filter] and others don't.

Hmm yes makes sense, i don't haev a string opinion on this suggestion then.

Or maybe we do it differently, deprecate all tags that expect SUF keys and subtags and add new versions of them that expect a [filter] subtag

No, don't deprectate this just becasue you like explicit filters better than 'inline' filters, iirc people did this in the past, also in the opposite direction (changed tags that required a [filter] to an inline filter, see for example in the changelog ans search for unpetrify), and that last things we want it it constantly changing api becasue of different taste of the developers over time.

@gfgtdf

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

that said, i think there are at least some tags ([recall], [terrain]) that have direct collision of keys (see the note about terrain= in [terrain] in the wiki, he collision of xy in unit filter and the destination in [recall] is probably less of an issue ), where some change could help. In particular if we do this for [filter] we shoudl probably also do this for location filters etc.

@jostephd

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

No, don't deprectate this just becasue you like explicit filters better than 'inline' filters, iirc people did this in the past, also in the opposite direction (changed tags that required a [filter] to an inline filter, see for example in the changelog ans search for unpetrify), and that last things we want it it constantly changing api becasue of different taste of the developers over time.

Don't throw the baby out with the bathwater. I agree that changing back and forth is bad, but that's not what I was suggesting. We should pick one style (either "with an explicit subtag", or "without an explicit subtag") and make it part of the style guide, so all new tags added to the WML language from this point on will use that one style. That style should probably be "with subtag" because of the collision problem you observed. For example, any addon that implements a custom WML action in Lua basically has to use "with subtag" already, if it wants to be forward compatible with future versions of wesnoth, because collisions.

I don't think the unpetrify example is analogous. That change was backward incompatible. My proposal is backward compatible. It will deprecate the current syntax but not remove it. There's no reason to remove it.

Note that I'm not saying anything about existing tags.

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