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

Don't send empty logs #732

Merged
merged 4 commits into from
Aug 15, 2018
Merged

Don't send empty logs #732

merged 4 commits into from
Aug 15, 2018

Conversation

NiallEgan
Copy link
Contributor

@NiallEgan NiallEgan commented Aug 14, 2018

This PR means that if the logs are the empty string or None, we no longer emit them on the socket. This stops extra empty lines being printed on the front end.


This change is Reviewable

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 2 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @NiallEgan)


aimmo-game/service.py, line 77 at r2 (raw file):

    for sid, avatar_id in session_id_to_avatar_id.iteritems():
        avatar_logs = logs_provider.get_user_logs(avatar_id)
        if avatar_logs is not None and avatar_logs != '':

We should print an empty string if that's what an avatar's code does right? Maybe we should ask James

Copy link
Contributor Author

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


aimmo-game/service.py, line 77 at r2 (raw file):

Previously, mrniket (Niket Shah) wrote…

We should print an empty string if that's what an avatar's code does right? Maybe we should ask James

Printing an empty string is the same thing as printing nothing at all. If the user did say print(''), this wouldn't print `` but \n - we'd still print that.

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


aimmo-game/service.py, line 77 at r2 (raw file):

Previously, NiallEgan (Niall Egan) wrote…

Printing an empty string is the same thing as printing nothing at all. If the user did say print(''), this wouldn't print `` but \n - we'd still print that.

Yeah, I would say that's the expected behaviour right? It's what happens if we run python in the terminal

Copy link
Contributor Author

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


aimmo-game/service.py, line 77 at r2 (raw file):

Previously, mrniket (Niket Shah) wrote…

Yeah, I would say that's the expected behaviour right? It's what happens if we run python in the terminal

Yeah, that's the behaviour we have here

Copy link
Contributor Author

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


aimmo-game/service.py, line 77 at r2 (raw file):

Previously, NiallEgan (Niall Egan) wrote…

Yeah, that's the behaviour we have here

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.

Reviewed 1 of 1 files at r3.
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.

:lgtm:

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

@NiallEgan NiallEgan merged commit b6ddf7c into master Aug 15, 2018
@NiallEgan NiallEgan deleted the dont_send_empty_logs branch August 15, 2018 09:56
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.

2 participants