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

AoI S1: Hint about recruiting a healer, if the player doesn't have any #3733

Merged
merged 1 commit into from Jan 6, 2019

Conversation

Projects
None yet
4 participants
@stevecotton
Copy link
Contributor

stevecotton commented Nov 18, 2018

This is just a hint on the general principle that it's good to have a healer,
based on the review comments of #3683.

When playing on easy, there's not much experience available in this first
scenario. Any hints about leveling units probably belong in S2 or later,
which also has the advantage of being able to talk about the enemies in S2.

AoI S1: Hint about recruiting a healer, if the player doesn't have any
This is just a hint on the general principle that it's good to have a healer.

When playing on easy, there's not much experience available in this first
scenario. Any hints about leveling units probably belong in S2 or later,
which also has the advantage of being able to talk about the enemies in S2.
@GregoryLundberg

This comment has been minimized.

Copy link
Contributor

GregoryLundberg commented Nov 18, 2018

.looks good on bench-check.will need cherry-pick to master.

@GregoryLundberg GregoryLundberg added this to the 1.14.6 milestone Nov 18, 2018

@jostephd

This comment has been minimized.

Copy link
Member

jostephd commented Nov 19, 2018

Looks good. It flows well with Erlornas' orders to Lomarfel.

In those orders, a connective is missing after "reinforcements". (not your fault)

The elf speaking the line speaks of "the battles ahead" but he doesn't know for a fact that there will be battles ahead. Maybe speak of "battles ahead" without an article?

@stevecotton

This comment has been minimized.

Copy link
Contributor

stevecotton commented Nov 21, 2018

The grammar looks okay to me, because "the battles ahead" are only "if we should embark on a campaign".

The reinforcements line also seems okay to me.

@jostephd

This comment has been minimized.

Copy link
Member

jostephd commented Nov 22, 2018

Well, syntactically, the reinforcements sentence is a comma splice. I assumed elves wouldn't use that pattern.

@sevu

This comment has been minimized.

Copy link
Member

sevu commented Jan 5, 2019

So… how shall the string be? (Or merging as is ?)

@jostephd

This comment has been minimized.

Copy link
Member

jostephd commented Jan 5, 2019

It's a good PR; I was just waiting for someone to offer a third (tie breaker) opinion on the two questions I raised.

@GregoryLundberg

This comment has been minimized.

Copy link
Contributor

GregoryLundberg commented Jan 5, 2019

On re-reading, I see a code comment about it not being the time, but if not, when is a good time since there is no other time? The message appears on turn 5 or not at all.

I don't see an issue with the present wording. I'd make a minor change, dropping "then" but that's mainly a personal preference.

What I do wonder about is why there is no change from the generic to the female message? Why not just use the generic (message=) form, then?

@jostephd

This comment has been minimized.

Copy link
Member

jostephd commented Jan 5, 2019

What I do wonder about is why there is no change from the generic to the female message? Why not just use the generic (message=) form, then?

I'm guessing it's because some languages would translate the sentence differently depending on the gender of the speaker.

@stevecotton

This comment has been minimized.

Copy link
Contributor

stevecotton commented Jan 5, 2019

Re the "wrong time to hint" comment - if the player has less than 5 surviving units by turn 5, I think they've already lost, and we'll get another chance to give them the hint on their next attempt at the scenario. Maybe a different hint should be shown, but 4hp/turn healing won't help much with whatever is going wrong.

@sevu sevu self-assigned this Jan 6, 2019

@jostephd

This comment has been minimized.

Copy link
Member

jostephd commented Jan 6, 2019

if the player has less than 5 surviving units by turn 5, I think they've already lost

Or they maybe they haven't recruited five units yet? A player have recruited four units or less to grab villages and hold a defensive line during the night (turn 5 is First Watch), intending to recruit the majority of his army on turn 6 in order to attack on turn 7 (dawn).

Anyway, 1.14.6 string freeze is coming up, and I don't think this issue should block the merge. Even if we agree changes are needed they can be made for 1.14.7.

@sevu sevu merged commit 2f68151 into wesnoth:1.14 Jan 6, 2019

3 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sevu

This comment has been minimized.

Copy link
Member

sevu commented Jan 6, 2019

merging for having the string in and to have to easier test the condition

@stevecotton stevecotton deleted the stevecotton:aoi_hint_recruit_healer branch Jan 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment