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

Fixed translation markup in unit::describe_builtin_effect() #932

Merged
merged 2 commits into from
Feb 17, 2017
Merged

Fixed translation markup in unit::describe_builtin_effect() #932

merged 2 commits into from
Feb 17, 2017

Conversation

gunchleoc
Copy link
Contributor

Replaced hard-coded word order and N_ for all strings in
unit::describe_builtin_effect() with vgettext/vngettext using
placeholders.

Replaced hard-coded word order and N_ for all strings in
unit::describe_builtin_effect() with vgettext/vngettext using
placeholders.
@gunchleoc
Copy link
Contributor Author

The diff is hard to read here, I found that git-cola did a better job on that.

@Vultraz
Copy link
Member

Vultraz commented Feb 15, 2017

Split diff view is easier.

@gfgtdf
Copy link
Contributor

gfgtdf commented Feb 15, 2017

hmm it seems like the code previousl used print_modifier which inserts a + before positive numbers to makew it clearer that that amounts gets added to the previous value

@gunchleoc
Copy link
Contributor Author

Not only previously - print_modifier is still used after my changes.

@CelticMinstrel
Copy link
Member

Hmm. Not really happy with the increased indentation level, though I can see why it was done... the stoi looks fishy but on closer inspection is actually fine... hmm...

Well, I guess I have no real objections to this. I could see a way to decrease the indentation a little, but dunno if it's worth it. (It's else if(!effect["increase"].empty()).)

@gfgtdf
Copy link
Contributor

gfgtdf commented Feb 16, 2017

ok didn't see it at first

@gunchleoc
Copy link
Contributor Author

I added the extra indentation level to get rid of lots of code duplication. The extra level is caused by hitpoints having a different effect than the rest. We could get rid of the extra indentation though if I did this:

const std::string &increase = (apply_to == "hitpoints") ? effect["increase_total"] : effect["increase"];

I'll be happy to change it if you prefer that.

@Vultraz
Copy link
Member

Vultraz commented Feb 16, 2017

Or you can also do if(increase.empty()) { return ""; }.

@CelticMinstrel
Copy link
Member

I don't see how Gunchleoc's suggestion would reduce the indentation level, given that attack also has a different effect... unless you're proposing calculating it redundantly even if the method is attack, in which case it would reduce the indentation level.

Vultraz's suggestion would definitely work.

@Vultraz Vultraz added this to the 1.13.7 milestone Feb 16, 2017
... for less indentation.
@gunchleoc
Copy link
Contributor Author

gunchleoc commented Feb 16, 2017

I implemented the early return now, which gains us 1 indentation level for the bottom part.

Attack has completely separate handling, so that indentation level has to stay like it was before.

I also added a comment to the diff.

t_string(N_("XP to advance"), "wesnoth");
}
} else if (apply_to == "max_attacks") {
} else {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can gain an indentation level here if we lose the "else" - IMO that would make the code structure a bit weird though.

@CelticMinstrel
Copy link
Member

Well, I don't have any objections to merging this.

@Vultraz Vultraz merged commit 7650eb7 into wesnoth:master Feb 17, 2017
@gunchleoc
Copy link
Contributor Author

Thanks for the review :)

@gunchleoc gunchleoc deleted the origin/bug-25469-ngettext branch February 17, 2017 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants