Skip to content
This repository has been archived by the owner on Aug 7, 2024. It is now read-only.

add custom exception for invalid action #789

Merged
merged 4 commits into from
Sep 6, 2018
Merged

Conversation

TheseusGrey
Copy link
Contributor

@TheseusGrey TheseusGrey commented Sep 5, 2018

Added custom exception to make console output more readable when user creates an invalid action.
Fixes #771


This change is Reviewable

Copy link
Contributor

@OlafSzmidt OlafSzmidt left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 7 files reviewed, 4 unresolved discussions (waiting on @dent50cent)

a discussion (no related file):
Should the pipfile changes (pylint) be a separate PR?



aimmo-game-worker/avatar_runner.py, line 62 at r2 (raw file):

handle_turn

handle_turn doesn't exist anymore?


aimmo-game-worker/avatar_runner.py, line 63 at r2 (raw file):

LOGGER.error(action)

process_avatar_turn is void when there's an error, what is this supposed to print?


aimmo-game-worker/avatar_runner.py, line 65 at r2 (raw file):

        LOGGER.error(action)
        if not isinstance(action, Action):
            raise InvalidActionException(action)

Where are we catching this? Printing it for user feedback?

Copy link
Contributor

@OlafSzmidt OlafSzmidt left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 7 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dent50cent)

Copy link
Contributor

@NiallEgan NiallEgan left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dent50cent)

a discussion (no related file):

Previously, OlafSzmidt (Olaf Szmidt) wrote…

Should the pipfile changes (pylint) be a separate PR?

This was done automatically by VS code. IMO we should be in the habit of automatically committing the Pipfile whenever changes are made to keep things up to date. (Also Niket said we should commit this in this PR).



aimmo-game-worker/avatar_runner.py, line 62 at r2 (raw file):

Previously, OlafSzmidt (Olaf Szmidt) wrote…
handle_turn

handle_turn doesn't exist anymore?

It does? Remember this is the user supplied avatar, not AvatarWrapper. We added a decide_action method because we though process_avatar_turn was getting too complex.


aimmo-game-worker/avatar_runner.py, line 63 at r2 (raw file):

Previously, OlafSzmidt (Olaf Szmidt) wrote…
LOGGER.error(action)

process_avatar_turn is void when there's an error, what is this supposed to print?

This should be removed


aimmo-game-worker/avatar_runner.py, line 65 at r2 (raw file):

Previously, OlafSzmidt (Olaf Szmidt) wrote…

Where are we catching this? Printing it for user feedback?

This is caught along with the rest of the exceptions in the catch block which surrounds it.

Copy link
Contributor

@OlafSzmidt OlafSzmidt left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @OlafSzmidt)

a discussion (no related file):

Previously, NiallEgan (Niall Egan) wrote…

This was done automatically by VS code. IMO we should be in the habit of automatically committing the Pipfile whenever changes are made to keep things up to date. (Also Niket said we should commit this in this PR).

Pipfile lock sure, but not dependency ranges imo. Resolving the discussion though


Copy link
Contributor

@OlafSzmidt OlafSzmidt left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


aimmo-game-worker/avatar_runner.py, line 62 at r2 (raw file):

Previously, NiallEgan (Niall Egan) wrote…

It does? Remember this is the user supplied avatar, not AvatarWrapper. We added a decide_action method because we though process_avatar_turn was getting too complex.

Ah yes, I forgot we didn't rename the handle_turn in the end

Copy link
Contributor

@OlafSzmidt OlafSzmidt left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dent50cent)

Copy link
Contributor Author

@TheseusGrey TheseusGrey left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @OlafSzmidt)


aimmo-game-worker/avatar_runner.py, line 63 at r2 (raw file):

Previously, NiallEgan (Niall Egan) wrote…

This should be removed

Done.

Copy link
Contributor

@OlafSzmidt OlafSzmidt left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@TheseusGrey TheseusGrey merged commit 7c23847 into master Sep 6, 2018
@TheseusGrey TheseusGrey deleted the catch_invalid_action branch September 6, 2018 08:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants