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.

Sign up for GitHub

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

Jump to bottom

feat(plugin): use MoveMistakes to construct InvalidMoveExceptions #321

Merged
merged 20 commits into from
Dec 15, 2020
Merged

feat(plugin): use MoveMistakes to construct InvalidMoveExceptions #321

merged 20 commits into from
Dec 15, 2020

Conversation

anarchuser
Copy link
Contributor

@anarchuser anarchuser commented Nov 3, 2020 •
edited
Loading

InvalidMoveExceptions are now being constructed with a MoveMistake and an optional Move, as opposed to String + optional Move.
This merges the previous ideas of returning MoveMistakes or throwing InvalidMoveExceptions from validation functions.
This in turn allows to cleanly distinguish the different exceptions from a limited set of mistake types, at the cost of arbitrary error messages.

Resolves #320

@anarchuser anarchuser self-assigned this Nov 3, 2020
xeruf
xeruf requested changes Nov 3, 2020
View reviewed changes
plugin/src/shared/sc/plugin2021/util/GameRuleLogic.kt Outdated Show resolved Hide resolved
plugin/src/shared/sc/plugin2021/util/GameRuleLogic.kt Outdated Show resolved Hide resolved
plugin/src/shared/sc/plugin2021/util/MoveMistake.kt Outdated Show resolved Hide resolved
plugin/src/shared/sc/plugin2021/util/MoveMistake.kt Outdated Show resolved Hide resolved
@anarchuser anarchuser changed the title Feat(plugin): give MoveMistakes more info on what happened feat(plugin): give MoveMistakes more info on what happened Nov 9, 2020
@anarchuser
Copy link
Contributor Author

The only issue that remains now is the inflexibility to pass information to InvalidMoveExceptions.
Should I turn MoveMistakes back into sealed classes and construct instances with individual parameters as I see fit?
Or will this just be a needless complication?

xeruf
xeruf requested changes Dec 1, 2020
View reviewed changes
plugin/src/shared/sc/plugin2021/util/GameRuleLogic.kt Outdated Show resolved Hide resolved
plugin/src/shared/sc/plugin2021/util/MoveMistake.kt Outdated Show resolved Hide resolved
sdk/src/framework/sc/shared/InvalidMoveException.kt Outdated Show resolved Hide resolved
SKoschnicke
SKoschnicke reviewed Dec 8, 2020
View reviewed changes
Copy link
Contributor

@SKoschnicke SKoschnicke left a comment

Choose a reason for hiding this comment

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

LGTM! Muss dafür etwas in der GUI angepasst werden?

@anarchuser anarchuser requested a review from xeruf December 8, 2020 15:52
@xeruf
Copy link
Member

xeruf commented Dec 11, 2020

@anarchuser rebase or squash?

@xeruf
Copy link
Member

xeruf commented Dec 11, 2020

if you squash, please make the commit message a little more descriptive

@anarchuser
Copy link
Contributor Author

squash and yes.
I have to look over it again though anyways, to adjust it to other changes we've merged recently

@anarchuser anarchuser marked this pull request as draft December 14, 2020 12:50
@xeruf
Copy link
Member

xeruf commented Dec 14, 2020

oof, I was about to announce its release. I thought it was merged already...

@xeruf xeruf marked this pull request as ready for review December 14, 2020 19:13
@xeruf
Copy link
Member

xeruf commented Dec 14, 2020

Why does it need adjusting though?

@anarchuser
Copy link
Contributor Author

nothing special, no, but there's a failing test

@xeruf
Copy link
Member

xeruf commented Dec 14, 2020

oh, and the gui doesn't display the message from the MoveMistake but simply "Regelverletzung" ^^

@anarchuser
style(plugin): renamed MoveMistake's file to match the class
bbc5040
@anarchuser
refactor(plugin): remove the internal msg of MoveMistakes
85da2a1
@anarchuser
feat(plugin): add a subjective toString to MoveMistakes
b7450e5 
This toString method returns a message telling which color violated and
why
@anarchuser
refactor(plugin): change the formulation for an obstruction mistake
b1b8db4
@anarchuser
refactor(plugin): turn enum into sealed class to store color info
722fa6a 
Since the different mistakes may need different information to be
passed, this seemed a sensible solution
@anarchuser
docs(plugin): add previous MoveMistake msg as documentation back in
44515be
@anarchuser
docs(plugin): reformat MoveMistakes docs to include OutOfBounds
7e8c990
@anarchuser
refactor(plugin): nest MoveMistake subclasses within the superclass
5f910c2
@anarchuser
Revert "refactor(plugin): turn enum into sealed class to store color …
e3401e2 
…info"

This reverts commit a5e1925.
@anarchuser
Revert "feat(plugin): add a subjective toString to MoveMistakes"
88c8fa7 
This reverts commit aca4a1e.
@anarchuser
refactor(sdk): create a MoveMistake interface
805ab54 
There will always be a distinctive set of types of invalid moves players can make.
The type of mistake that happened should be easily communicatable.
@anarchuser
refactor(sdk): pass MoveMistake instead of String on invalid moves
74d425e 
This allows for less customisability for InvalidMoveExceptions but makes
the type of exceptions clear and potentially reduces redundancy
@anarchuser
feat(sdk): store the MoveMistake enum within InvalidMoveExceptions
c9d6201 
With this, automated detection of the type of mistake that happened is
possible
@anarchuser
fix(plugin): add MoveMistake instance for unrecognised move formats
f86fa67
@anarchuser
feat(plugin): enforce move validation throwing instead of returning
0836804 
Reverts 38721b1
@anarchuser
docs(sdk): add description for InvalidMoveException
a6e5087
@anarchuser
docs(plugin): add line break to MoveMistake's description
dd48dd0
@anarchuser
docs(plugin): comment on behaviour of validate functions
764f279
@anarchuser
test(plugin): adapt to new validate function syntax
032aad4
@anarchuser
test(server): add TestMoveMistake to construct InvalidMoveExceptions
87acbcc 
Due to the changes to InvalidMoveExceptions it doesn't take a string
anymore. Thus, the Test classes need their own subset of MoveMistakes
@anarchuser
Copy link
Contributor Author

I fixed the failing tests (nothing broken, I just needed to adapt them to the changes).
So once all tests succeed, this can be squashed

@anarchuser anarchuser changed the title feat(plugin): give MoveMistakes more info on what happened feat(plugin): use MoveMistakes to construct InvalidMoveExceptions Dec 15, 2020
@anarchuser anarchuser merged commit d2e3d58 into master Dec 15, 2020
@anarchuser anarchuser deleted the feat/moveMistake branch December 15, 2020 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SKoschnicke SKoschnicke SKoschnicke approved these changes

@xeruf xeruf xeruf approved these changes

Assignees

@anarchuser anarchuser

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MoveMistakes don't tell who violated
3 participants
@anarchuser @xeruf @SKoschnicke