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

Update EmitInfantryOnSell.cs per open issue #15077 #15161

Merged
merged 1 commit into from May 31, 2018

Conversation

Projects
None yet
8 participants
@zmudaj
Copy link
Contributor

zmudaj commented May 20, 2018

Per open issue #15077 : " having ActorTypes: e1 as a default of EmitInfantryOnSell: is inconsistent and should therefore be removed."

Removed the default setting for ActorTypes (leaving it uninitialized) and removed the reference to the default in the description

Thank you for your contribution to OpenRA!

Please be aware that we do not have enough project maintainers to match the rate of contributions, so it may take several days before somebody is able to respond to your Pull Request.

You can help speed up the review process by following a few steps:

  • Make sure that you have read and understand the OpenRA Coding Standard (see https://github.com/OpenRA/OpenRA/wiki/Coding-Standard).
  • Write quality commit messages (see https://chris.beams.io/posts/git-commit/).
  • Only commit changes that directly relate to your Pull Request. Use your Git interface to unstage any unrelated changes to project files, line endings, whitespace, or other files.
  • Review the code diff view below to double check that your changes satisfy the above three points.
  • Use the make test and make check commands to check for (and fix!) any issues that are reported by our automated tests.
  • If you are changing shared mod or engine code, make sure that you have tested your changes in all four default mods.
  • Respond to review comments as soon as you reasonably can. Reviewers will usually prioritize Pull Requests that are still fresh in their minds. Make sure to leave a comment when you push new changes, otherwise GitHub does not automatically notify reviewers!
  • Leave a polite comment asking for reviews if a week or more has passed without feedback.

If you need any help you can ask in the #openra IRC channel on freenode (most active during European evenings).

@ABrandau

This comment has been minimized.

Copy link
Contributor

ABrandau commented May 20, 2018

I think you might also want to add an upgrade rule for custom mods, and define the emitted infantry on the official mods yamls.

@abcdefg30
Copy link
Member

abcdefg30 left a comment

Hi, and thanks for the pull request. ^^

This looks good from a first glance, however we'll want an update rule for this change. Do you want to take a shot at adding one? We can try to walk you through (on IRC) or I could provide the update rule if you want. Edit: Ah well, seems like @ABrandau was faster than I was. ^^

[Desc("Be sure to use lowercase. Default value is \"e1\".")]
public readonly string[] ActorTypes = { "e1" };
[Desc("Be sure to use lowercase.")]
public readonly string[] ActorTypes;

This comment has been minimized.

@abcdefg30

abcdefg30 May 20, 2018

Member

It would be good if you add a [FieldLoader.Require] attribute to this field.

This comment has been minimized.

@zmudaj

zmudaj May 21, 2018

Author Contributor

Thanks for the feedback. @abcdefg30 where you saying @ABrandau already added the update rule? If not, I'd like to take a shot at it

This comment has been minimized.

@matjaeck

matjaeck May 21, 2018

Contributor

He meant that he answered too late. See this commit for an example update rule. You also might want to check out Contributing, assuming that you haven't set up an IDE or git client yet.

@ABrandau

This comment has been minimized.

Copy link
Contributor

ABrandau commented May 21, 2018

The point with the update rule is that your PR removes the light infantry as default unit for the trait. If someone merges your PR, selling buildings will not spawn infantry, and that is not intended, so i believe you have to correct that (I am not a dev so whatever abcdfg or anyone else says has more priority than what I say)

First you should do the manual changes for the official mods, basically look for the EmmitInfantryOnSell trait on defaults and set the e1 back in, and then make the pdate rule, I have no clue about that.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented May 21, 2018

First you should do the manual changes for the official mods...

The recommended procedure is to write an update rule first, and then run that on the default mods in order to change their yaml. Doing the changes manually and then writing a rule that may or may not replicate those results is not a good idea.

@MunWolf

This comment has been minimized.

Copy link
Contributor

MunWolf commented May 24, 2018

What you want to do is look for any EmitInfantryOnSell yaml nodes on actors, check if they have a ActorTypes subnode and if the do not add one in that defines e1 as the actor type.

@MunWolf

This comment has been minimized.

Copy link
Contributor

MunWolf commented May 24, 2018

As a sidenote, why is this called EmitInfantry when it works on Actors?

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented May 24, 2018

Artifact.

The WW counterpart is limited to infantry.

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented May 31, 2018

Since #15171 takes care of the rest, we can take this as-is. Thanks for your contribution!

@reaperrr reaperrr merged commit 2cf3c48 into OpenRA:bleed May 31, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented May 31, 2018

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