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

Fix for text describing the FACTION command, in the presence of MARTIAL #157

Merged

Conversation

nedbrek
Copy link
Contributor

@nedbrek nedbrek commented Nov 25, 2023

No description provided.

@artyomtrityak
Copy link
Member

snapshot failed, can you please update it

f << enclose(class_tag("div", "rule"), true) << '\n' << enclose("div", false);
f << anchor("faction") << '\n';

const bool martial_faction = Globals->FACTION_ACTIVITY == FactionActivityRules::MARTIAL_MERGED;
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this test is slightly wrong as it's resulting in the neworigins code generating rules which reference war/trade/magic rather than martial/magic.

Copy link
Contributor

Choose a reason for hiding this comment

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

you need to add parens around the (Globals->FACTION_ACTIIVITY == )..

Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh.. I'm an idiot.. the code is correct.. the snapshot test is diffing backwards which led me to read it wrong.. I will fix that in one of my upcoming commits.. Regardless you need to regenerate the rules snapshots and include them as part of the diff in order to allow the tests to pass.

<< example_end();

if (martial_faction)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

I strongly dislike having open braces on new lines. it's just a waste of space and does not (imho) improve readability of the code.. please use

if (condition) {
} else {
}

:)

@jt-traub jt-traub changed the base branch from master to jt-cleanup-various-prs May 4, 2024 23:05
@jt-traub
Copy link
Contributor

jt-traub commented May 4, 2024

I am pulling this into an upcoming merge which will clean up/complete a bunch of other PRs (this is being merged into my working branch for that and this PR closed)

@jt-traub jt-traub merged commit c1d6d72 into Atlantis-PBEM:jt-cleanup-various-prs May 4, 2024
4 of 5 checks passed
@jt-traub
Copy link
Contributor

jt-traub commented May 5, 2024

Changes are in PR #186

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.

3 participants