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

Refactor combat trainer processes #379

Merged
merged 13 commits into from
May 11, 2016
Merged

Refactor combat trainer processes #379

merged 13 commits into from
May 11, 2016

Conversation

rpherbig
Copy link
Owner

@rpherbig rpherbig commented May 9, 2016

Combat-trainer is duplicating default values for settings when:

  • Defining each setting in base.yaml
  • The settings are read in CombatTrainer::make_processes
  • The named params are passed to each individual process

@rcuhljr 's suggestion was to pass the settings into each process and let initialize unpack them. That's a good step forward.

There is still duplication between SetupProcess and base.yaml, but less
duplication than there was
arrange_count: arrange_count,
skin: settings.skinning['skin'],
arrange_all: settings.skinning['arrange_all'],
arrange_count: settings.skinning['arrange_count'],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we might have problems here for people who delete the skinning settings or something? Not sure but this seemed red flag to me.

Copy link
Owner Author

Choose a reason for hiding this comment

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

This should be addressed once I move LootProcess over.

Copy link
Owner Author

Choose a reason for hiding this comment

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

If they delete skinning, then they'll get the default in base.yaml. If they override it (such as skinning:), then yeah, we might have a problem. But I'll change it to settings.skinning({}).

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

Successfully merging this pull request may close these issues.

2 participants