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

Remove non user code traceback #797

Merged
merged 13 commits into from
Sep 6, 2018
Merged

Conversation

TheseusGrey
Copy link
Contributor

@TheseusGrey TheseusGrey commented Sep 6, 2018

Formatted traceback so the user's console only shows output relevant to their code.


This change is Reviewable

Paris Goldman-Smith added 7 commits September 6, 2018 09:47
Console output for errors now only shows the traceback for the users code, just need to fix minor issue
code should now be simpler and more readable
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.

I think we could do with a third test, one which tests if the traceback is fine if they return None etc.

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


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

        except InvalidActionException as e:
            print(str(e))

I think that if you call print then str will be automatically called, so maybe the str is redundant? Also maybe we should have a comment explaining why we have a separate except?


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

        return action.serialise()

    def format_user_traceback(self, tb_list):

This can be a static method


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

        return action.serialise()

    def format_user_traceback(self, tb_list):

I think a better name would be something like get_only_user_traceback?


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

        start_of_user_traceback = 0
        for i in range(len(tb_list)):
            if tb_list[i].__contains__('<string>'):

I think you should use the in keyword here. In general, you shouldn't invoke magic methods directly, use in, +, or etc


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

        runner = AvatarRunner()
        response = runner.process_avatar_turn(world_map={}, avatar_state={}, src_code=avatar)
        self.assertFalse(response['log'].__contains__('/usr/src/app/'))

__contains__


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

        runner = AvatarRunner()
        response = runner.process_avatar_turn(world_map={}, avatar_state={}, src_code=avatar)
        self.assertFalse(response['log'].__contains__('/usr/src/app/'))

__contains__

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.

I'd suggest to try to briefly describe the sort of "various"/ "more" changes as it is a bit easier to review

Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (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: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @dent50cent and @NiallEgan)


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

Previously, NiallEgan (Niall Egan) wrote…

I think that if you call print then str will be automatically called, so maybe the str is redundant? Also maybe we should have a comment explaining why we have a separate except?

These have been fixed, the examples i found when writing the code were for python 2.5 or lower, did more research and found it had be fixed for python 2.6. Added comment explaining the addition except.


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

Previously, NiallEgan (Niall Egan) wrote…

This can be a static method

Done.


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

Previously, NiallEgan (Niall Egan) wrote…

I think a better name would be something like get_only_user_traceback?

Done


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

Previously, NiallEgan (Niall Egan) wrote…

I think you should use the in keyword here. In general, you shouldn't invoke magic methods directly, use in, +, or etc

Done


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

Previously, NiallEgan (Niall Egan) wrote…

__contains__

Done.


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

Previously, NiallEgan (Niall Egan) wrote…

__contains__

Done.

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 2 files at r2.
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

@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, 1 unresolved discussion (waiting on @dent50cent)


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

        except Exception as e:
            traceback_list = traceback.format_exc().split('\n')

Maybe this should be in get_only_user_traceback?

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: all files reviewed, 1 unresolved discussion (waiting on @NiallEgan)


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

Previously, NiallEgan (Niall Egan) wrote…

Maybe this should be in get_only_user_traceback?

Done

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: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @NiallEgan and @dent50cent)


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

    @staticmethod
    def get_only_user_traceback(e):

Do we have to pass e in here?

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: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @NiallEgan and @dent50cent)


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

Previously, NiallEgan (Niall Egan) wrote…

Do we have to pass e in here?

no, this has been fixed.

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 1 of 1 files at r3, 1 of 1 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@TheseusGrey TheseusGrey merged commit 737b4a3 into master Sep 6, 2018
@TheseusGrey TheseusGrey deleted the remove_non_user_code_traceback branch September 6, 2018 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants