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

Fix local non -k mode #695

Merged
merged 2 commits into from
Aug 1, 2018
Merged

Fix local non -k mode #695

merged 2 commits into from
Aug 1, 2018

Conversation

OlafSzmidt
Copy link
Contributor

@OlafSzmidt OlafSzmidt commented Jul 30, 2018

Fixes #694. Please read the issue if you're reviewing.


This change is Reviewable

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


aimmo/game_renderer.py, line 53 at r1 (raw file):

def _connection_on_k8s_mode(game_port):

Maybe a comment to explain why this function works? Could be a bit confusing in a few months time

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


aimmo/game_renderer.py, line 53 at r1 (raw file):

Previously, NiallEgan (Niall Egan) wrote…

Maybe a comment to explain why this function works? Could be a bit confusing in a few months time

@mrniket told me not to yesterday, I'm for docstringing this too, it's quite a complicated method to understand in terms of how the architecture works.

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


aimmo/game_renderer.py, line 53 at r1 (raw file):

Previously, OlafSzmidt (Olaf Szmidt) wrote…

@mrniket told me not to yesterday, I'm for docstringing this too, it's quite a complicated method to understand in terms of how the architecture works.

@mrniket ???

I would prefer making the code "clean" over comments. Comments are more easily forgotten about when you are modifying this code later, so are more likely to be outdated. I would argue that in this case, the function name contains the same information a comment would have done but via code.

For more information I recommend having a quick look at these links:

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 1 files reviewed, all discussions resolved (waiting on @OlafSzmidt)


aimmo/game_renderer.py, line 53 at r1 (raw file):

Previously, mrniket (Niket Shah) wrote…

I would prefer making the code "clean" over comments. Comments are more easily forgotten about when you are modifying this code later, so are more likely to be outdated. I would argue that in this case, the function name contains the same information a comment would have done but via code.

For more information I recommend having a quick look at these links:

Ok, agreed that a comment won't add much. I think we could do with a better way to check if we are using k8s, but that is probably outside the scope of this PR.

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 r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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: :shipit: complete! all files reviewed, all discussions resolved


aimmo/game_renderer.py, line 53 at r1 (raw file):

Ok, agreed that a comment won't add much. I think we could do with a better way to check if we are using k8s, but that is probably outside the scope of this PR.

I agree it's not ideal and intuitive, but changes would require both the front-end and backend to be altered.

@OlafSzmidt OlafSzmidt merged commit 2937452 into master Aug 1, 2018
@OlafSzmidt OlafSzmidt deleted the fix_local_mode branch August 1, 2018 09:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants