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

resolve Check whether special is active by tag name or by id (bis) #4424

Closed
wants to merge 21 commits into from

Conversation

@newfrenchy83
Copy link
Contributor

commented Oct 3, 2019

replace #3920 PR for resolve #3915

newfrenchy83 added 3 commits Oct 3, 2019
replace #3920 PR
@CelticMinstrel

This comment has been minimized.

Copy link
Member

commented Oct 8, 2019

Just noting that I don't see any problems with this; it probably can be merged.

Copy link
Member

left a comment

Thanks for the pull request and for the getting the indentation correct.

I have a question about a possible incompatible change, see below.

Do you already have WML or Lua code that you used to test this PR when you developed it? If so, could you share it please? (I'm talking merely about doing ad-hoc testing, interactively, nothing more than that.)

@@ -73,7 +73,7 @@ class attack_type : public std::enable_shared_from_this<attack_type>

// In unit_abilities.cpp:

bool get_special_bool(const std::string& special, bool simple_check=false) const;
bool get_special_bool(const std::string& special, bool simple_check=false, bool special_id=true, bool special_tags=true) const;

This comment has been minimized.

Copy link
@jostephd

jostephd Oct 12, 2019

Member

Please document the two new parameters:

/// @return True iff the special @a special is active.
/// @param special_id If true, match @a special against the @c id of special tags.
/// @param special_tags If true, match @a special against the tag name of special tags.

This comment has been minimized.

Copy link
@jostephd
src/units/attack_type.cpp Show resolved Hide resolved
@newfrenchy83

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2019

i have put a test case in https://github.com/wesnoth/wesnoth/pull/3920/files but the filter_weapon in drains abilities is in wrong place but i must wait what #4435 will be merged before to put a test case with true code in this PR.

@jostephd

This comment has been minimized.

Copy link
Member

commented Oct 12, 2019

i have put a test case in https://github.com/wesnoth/wesnoth/pull/3920/files but the filter_weapon in drains abilities is in wrong place but i must wait what #4435 will be merged before to put a test case with true code in this PR.

Thanks. I understand that #4435 blocks adding that test case to this PR, but couldn't we merge this PR before #4435? I realize the test case in #3920 wouldn't work yet, but we could test the feature differently to be sure it works. For example, we could use a StandardUnitFilter with a [has_attack] subtag.

src/units/attack_type.cpp Outdated Show resolved Hide resolved
@@ -164,6 +165,7 @@ static bool matches_simple_filter(const attack_type & attack, const config & fil


if(!filter_special_active.empty()) {
deprecated_message("special_active=", DEP_LEVEL::PREEMPTIVE, {1, 16, 0}, "Please use special_id_active or special_type_active instead");

This comment has been minimized.

Copy link
@jostephd

jostephd Oct 12, 2019

Member

Looks good, but the version should be {1, 17, 0}.

This comment has been minimized.

Copy link
@newfrenchy83

newfrenchy83 Oct 12, 2019

Author Contributor

it is done

This comment has been minimized.

Copy link
@jostephd

jostephd Oct 12, 2019

Member

You need to include deprecation.hpp and game_version.hpp to fix a build error.

@jostephd

This comment has been minimized.

Copy link
Member

commented Oct 13, 2019

@newfrenchy83 With this PR, I started HttT S6 The Siege of Elensefar in debug mode and ran the following commands in the lua console:

$ wesnoth.get_units { wml.tag.has_attack { special = 'magical' }  }
{[1]=unit: 0x5587827722d8}
$ wesnoth.get_units { wml.tag.has_attack { special_id = 'magical' }  }
{}
$ wesnoth.get_units { wml.tag.has_attack { special_type = 'chance_to_hit' }  }
{[1]=unit: 0x55879f520158}

Why did the second one return nothing?

@newfrenchy83

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2019

@jostephd i fixed special_id and corrected the test case.

@newfrenchy83

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2019

@newfrenchy83 With this PR, I started HttT S6 The Siege of Elensefar in debug mode and ran the following commands in the lua console:

$ wesnoth.get_units { wml.tag.has_attack { special = 'magical' }  }
{[1]=unit: 0x5587827722d8}
$ wesnoth.get_units { wml.tag.has_attack { special_id = 'magical' }  }
{}
$ wesnoth.get_units { wml.tag.has_attack { special_type = 'chance_to_hit' }  }
{[1]=unit: 0x55879f520158}

Why did the second one return nothing?

@newfrenchy83 With this PR, I started HttT S6 The Siege of Elensefar in debug mode and ran the following commands in the lua console:

$ wesnoth.get_units { wml.tag.has_attack { special = 'magical' }  }
{[1]=unit: 0x5587827722d8}
$ wesnoth.get_units { wml.tag.has_attack { special_id = 'magical' }  }
{}
$ wesnoth.get_units { wml.tag.has_attack { special_type = 'chance_to_hit' }  }
{[1]=unit: 0x55879f520158}

Why did the second one return nothing?

I haven't changed special to special_id in code, this is fixed now

Copy link
Member

left a comment

Tested, both in HttT S6 as before and with the new wesnoth -t tests. Looks good.

Requesting minor changes to the test scenario.

Do you have any further changes planned as part of this PR? If not, I'll do a final review pass. If all goes well, I hope to merge this before 1.15.2.

Thing needed after merge:

data/scenario-test.cfg Outdated Show resolved Hide resolved
data/scenario-test.cfg Outdated Show resolved Hide resolved
data/scenario-test.cfg Outdated Show resolved Hide resolved
data/scenario-test.cfg Show resolved Hide resolved
@newfrenchy83

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2019

@jostephd it is done you can doing final review pass

@jostephd jostephd self-requested a review Oct 15, 2019
@jostephd

This comment has been minimized.

Copy link
Member

commented Oct 15, 2019

@newfrenchy83 Please address #4424 (comment). With that changed, I would be happy to merge this.

src/units/abilities.cpp Outdated Show resolved Hide resolved
@newfrenchy83

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2019

@jostephd i put doxygen documentation in attack_type.cpp

@jostephd

This comment has been minimized.

Copy link
Member

commented Oct 16, 2019

@jostephd i put doxygen documentation in attack_type.cpp

I don't think that would work. I think doxygen only recognizes comments in function declarations and definitions. What's more, you deleted the @ from @param, so doxygen won't recognize it anyway.

I'll fix it and merge.

@jostephd

This comment has been minimized.

Copy link
Member

commented Oct 16, 2019

Merged in a87be48.

@newfrenchy83 Please address the first two items of #4424 (review). And thank you very much for the pull request and for being so patient with how long it took us to review and merge it! 👍

@jostephd jostephd closed this Oct 16, 2019
@newfrenchy83

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2019

Merged in a87be48.

@newfrenchy83 Please address the first two items of #4424 (review). And thank you very much for the pull request and for being so patient with how long it took us to review and merge it! +1

@jostephd i don't know how doing that

@newfrenchy83 newfrenchy83 deleted the newfrenchy83:patch-15 branch Oct 17, 2019
@jostephd

This comment has been minimized.

Copy link
Member

commented Oct 17, 2019

@newfrenchy83 I went ahead and made the change myself, see https://wiki.wesnoth.org/index.php?title=FilterWML&diff=64960&oldid=60669. Please review that, is the documentation correct? Did I leave anything out?

@newfrenchy83

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2019

it is correct for me

@jostephd

This comment has been minimized.

Copy link
Member

commented Oct 17, 2019

Thanks for the review!

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.