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

Fix #673 - error catching bug #674

Merged
merged 4 commits into from
Jul 18, 2018
Merged

Fix #673 - error catching bug #674

merged 4 commits into from
Jul 18, 2018

Conversation

OlafSzmidt
Copy link
Contributor

@OlafSzmidt OlafSzmidt commented Jul 18, 2018

As described in #673. Does some changes to how we catch errors, refactors method names and (please check) flips method params around because I thought they were wrong.


This change is Reviewable

@OlafSzmidt OlafSzmidt added the bug label Jul 18, 2018
@OlafSzmidt OlafSzmidt self-assigned this Jul 18, 2018
@OlafSzmidt OlafSzmidt added this to the Sprint 66 Reactive Console milestone Jul 18, 2018
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: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @OlafSzmidt)


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

        self.avatar = avatar

    def process_avatar_turn(self, world_map, avatar_state):

Agree that this is the right way round.


aimmo-game-worker/tests/test_avatar_runner.py, line 10 at r1 (raw file):

    def test_runner_does_not_crash_on_code_errors(self):
        class Avatar(object):
            def process_avatar_turn(self, world_map, avatar_state):

Should this not still be handle_turn, as this is an Avatar not an AvatarRunner.

Copy link
Contributor

@mrniket mrniket 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 3 files reviewed, 3 unresolved discussions (waiting on @OlafSzmidt)


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

                from avatar import Avatar
                self.avatar = Avatar()
            return self.avatar.handle_turn(world_map, avatar_state)

was this switch made to reflect the dummy avatar code? maybe we also need to check our teaching materials too if you are changing this

Copy link
Contributor

@riaJha97 riaJha97 left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @riaJha97 and @OlafSzmidt)


aimmo-game-worker/tests/test_avatar_runner.py, line 10 at r1 (raw file):

Previously, NiallEgan (Niall Egan) wrote…

Should this not still be handle_turn, as this is an Avatar not an AvatarRunner.

Yeah I agree, it should still be called handle_turn()

Copy link
Contributor Author

@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: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @riaJha97, @NiallEgan, and @OlafSzmidt)


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

Previously, NiallEgan (Niall Egan) wrote…

Agree that this is the right way round.

Good! :)


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

Previously, mrniket (Niket Shah) wrote…

was this switch made to reflect the dummy avatar code? maybe we also need to check our teaching materials too if you are changing this

Our dummy avatar code is:

    def handle_turn(self, world_view, events):

Which is even worse


aimmo-game-worker/tests/test_avatar_runner.py, line 10 at r1 (raw file):

Previously, NiallEgan (Niall Egan) wrote…

Should this not still be handle_turn, as this is an Avatar not an AvatarRunner.

Ah, of course... Done.

Copy link
Contributor

@mrniket mrniket 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: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @riaJha97, @NiallEgan, and @mrniket)


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

Previously, OlafSzmidt (Olaf Szmidt) wrote…

Our dummy avatar code is:

    def handle_turn(self, world_view, events):

Which is even worse

and the teaching materials?

Copy link
Contributor

@mrniket mrniket 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: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @riaJha97, @NiallEgan, and @OlafSzmidt)


aimmo-game-worker/tests/test_avatar_runner.py, line 15 at r2 (raw file):

        runner = AvatarRunner(Avatar())
        action = runner.process_avatar_turn({}, {})

maybe an improvement on this is to include the names of the parameters. This might just be a personal preference though

Copy link
Contributor Author

@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: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @riaJha97, @NiallEgan, and @OlafSzmidt)


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

Previously, mrniket (Niket Shah) wrote…

and the teaching materials?

Teaching materials are avatar_state, world_map. But I'm still not convinced that's what our code is.

Copy link
Contributor

@CelineBoudier CelineBoudier left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @NiallEgan and @mrniket)


aimmo-game-worker/tests/test_avatar_runner.py, line 15 at r2 (raw file):

Previously, mrniket (Niket Shah) wrote…

maybe an improvement on this is to include the names of the parameters. This might just be a personal preference though

might make it more readable indeed

Copy link
Contributor Author

@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: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @CelineBoudier, @NiallEgan, and @mrniket)


aimmo-game-worker/tests/test_avatar_runner.py, line 15 at r2 (raw file):

Previously, CelineBoudier (Celine Boudier) wrote…

might make it more readable indeed

Done;)

Copy link
Contributor

@mrniket mrniket 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: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @CelineBoudier, @NiallEgan, and @mrniket)


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

Previously, OlafSzmidt (Olaf Szmidt) wrote…

Teaching materials are avatar_state, world_map. But I'm still not convinced that's what our code is.

So if you switch these two around, we will have to change the teaching materials. If you don't, we change our example avatars' code. However, as you pointed out the example avatars code is already wrong so I would just change the example avatars' code and not this or the teaching materials

Copy link
Contributor

@mrniket mrniket left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @NiallEgan and @OlafSzmidt)

Copy link
Contributor Author

@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, 2 unresolved discussions (waiting on @NiallEgan and @mrniket)


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

Previously, mrniket (Niket Shah) wrote…

So if you switch these two around, we will have to change the teaching materials. If you don't, we change our example avatars' code. However, as you pointed out the example avatars code is already wrong so I would just change the example avatars' code and not this or the teaching materials

Done.

Copy link
Contributor

@riaJha97 riaJha97 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, 4 of 4 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @NiallEgan and @mrniket)

Copy link
Contributor

@mrniket mrniket 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 @NiallEgan)

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.

Reviewed 2 of 3 files at r1, 1 of 1 files at r3, 4 of 4 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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.

:lgtm:

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

Copy link
Contributor

@mrniket mrniket 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

@OlafSzmidt OlafSzmidt merged commit cea04e2 into master Jul 18, 2018
@OlafSzmidt OlafSzmidt deleted the quickfix_673 branch July 18, 2018 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants