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

Implement new selected move types #1316

Merged
merged 2 commits into from Nov 8, 2018

Conversation

4 participants
@chrisgilmerproj
Contributor

chrisgilmerproj commented Nov 7, 2018

Description

Previously we had a move type named COMBO that was supposed to represent PPM + HHG moves. However, there could be lots of combo moves and they could include more or different types. This change lists the moves we know about and creates a new move type HHG_PPM following a set of rules listed in the code.

In addition, I've removed instances of internalmessages.SelectedMoveType from our DB's model layer since it's not a good practice and instead created a new SelectedMoveType that is 1:1 with our API.

Reviewer Notes

Make sure the documentation around combo moves makes sense to you please.

Setup

Run server tests or e2e tests, there should be no noticeable difference.

Code Review Verification Steps

  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

@chrisgilmerproj chrisgilmerproj self-assigned this Nov 7, 2018

// This lists available move types in the system
// Combination move types like HHG+PPM should be added as an underscore separated list
// The list should be lexigraphically sorted. Ex: UB + PPM will always be 'PPM_UB'

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Nov 7, 2018

Contributor

Please let me know if this makes sense or if it could be expanded to be clearer.

@kahlouie

Just the one open question about naming :)

- UB
- POV
- NTS
- HHG_PPM

This comment has been minimized.

@kahlouie

kahlouie Nov 8, 2018

Contributor

I have been opting to use the slightly more verbose hhgAndPpm on the frontend for the flag, and am wondering if it's more clear without as much explanation. Either way I think it might be better to standardize them. The flag, is hopefully short lived, and naming is hard, so this is not a blocker, but rather an open question.

This comment has been minimized.

@jim

jim Nov 8, 2018

Contributor

I think it's probably OK for them to be different in this case, although I agree with you that in general keeping things consistent is good.

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Nov 8, 2018

Contributor

I agree that it's probably ok to be different here.

@jim

jim approved these changes Nov 8, 2018

🚢

@chrisgilmerproj chrisgilmerproj merged commit a7c7c8d into master Nov 8, 2018

9 checks passed

ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details

@chrisgilmerproj chrisgilmerproj deleted the cg_161796830_implement_new_move_types branch Nov 8, 2018

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