General code cleanup #507

eforgacs · 2019-11-29T22:23:24Z

Removing unnecessary parentheses, test for membership should be "not in", typo fixes, indentation/spacing, changing from use of deprecated functions to non-deprecated functions, capitalization, fixing syntax of Bash scripts, line length, added "await" to coroutine that is not awaited.

norraxx · 2019-12-02T03:44:33Z

tests/unit_tests/test_decorators.py

@@ -7,7 +7,7 @@

 def test_with_logger():
    @with_logger
-    class testclass():
+    class testclass:


Can be camel case

Thanks! Just changed it.

Removing unnecessary parentheses, test for membership should be "not in", typo fixes, indentation/spacing, changing from use of deprecated functions to non-deprecated functions, capitalization, fixing syntax of Bash scripts, line length, added "await" to coroutine that is not awaited. Changes in this commit: - Making TestClass use CamelCase (tests/unit_tests/test_decorators.py)

Askaholic

If you wanted to you could run a few of these files through black and see if it produces anything sane.

Askaholic · 2019-12-02T03:53:30Z

scripts/pipenv_run_tests.sh

@@ -1,2 +1,2 @@
 #! /bin/bash
-docker exec -i faf-db mysql faf < test-data.sql && scripts/run_tests_with_coverage.sh $@
+docker exec -i faf-db mysql faf < test-data.sql && scripts/run_tests_with_coverage.sh "$@"


Are you sure this will still work? The point is to forward all command line arguments to pytest so that for example you can do pipenv run tests tests/integration_tests -s --cli-log-level 5. I would think that adding quotes would mash all of your arguments into a single string, which presumably doesn't get parsed correctly.

Here's the warning from ShellCheck that I get when I leave it in its current state:

"Double quote array expansions to avoid re-splitting elements."

Rationale:
Double quotes around $@ (and similarly, ${array[@]}) prevents globbing and word splitting of individual elements, while still expanding to multiple separate arguments.

Let's say you have three arguments: baz, foo bar and *

"$@" will expand into exactly that: baz, foo bar and *

$@ will expand into multiple other arguments: baz, foo, bar, file.txt and otherfile.jpg

Since the latter is rarely expected or desired, ShellCheck warns about it.

https://github.com/koalaman/shellcheck/wiki/SC2068

I just tried it on my machine, and it seems to still work so I guess there's no problem.

eforgacs · 2019-12-02T04:08:37Z

@Askaholic I had never heard about black until now - is this the right link?

https://black.readthedocs.io/en/stable/

I really like the description! Can't wait to try it out!

norraxx · 2019-12-02T03:45:52Z

tests/unit_tests/test_matchmaker_queue.py

@@ -227,7 +227,7 @@ def matchmaker_players_all_match(player_factory):
    matchmaker_queue._matches = deque(matches)

    async def call_shutdown():
-        asyncio.sleep(1)
+        await asyncio.sleep(1)


Is that await needed? If that code worked before it didn't wait for sleep anyway

I guess not. TBH I'm not sure any more why I was calling shutdown like that.

norraxx · 2019-12-02T04:30:28Z

server/__init__.py

@@ -121,7 +121,8 @@ def report_dirties():
                # allowed to see them.
                if game.visibility == VisibilityState.FRIENDS:
                    # To see this game, you must have an authenticated connection and be a friend of the host, or the host.
-                    validation_func = lambda lobby_conn: lobby_conn.player.id in game.host.friends or lobby_conn.player == game.host
+                    validation_func = lambda \
+                        lobby_conn: lobby_conn.player.id in game.host.friends or lobby_conn.player == game.host


Imho, you should leave first lambda param on same line as lambda. How do you read code? I read first input params then logic

That's a good suggestion. I think this was a "line too long" formatting change, so I can change this to be on one line.

How about just defining the function with a def? I don't see the point of using a lambda function here anyways.

norraxx · 2019-12-02T04:32:10Z

+1 for black, but if we will use black, we need to reformat whole project.

Askaholic · 2019-12-14T21:43:14Z

server/__init__.py

-    nts_client: Optional[TwilioNTS],
-    geoip_service: GeoIpService,
-    ladder_service: LadderService
+        address: (str, int),


What's with the double indents? I prefer single, and for what it's worth so does black

That's a good question. PyCharm formatted it that way, I think it's just conforming to PEP8, but I'm amenable to either single or double, no strong feelings one way or the other.

Ok, let's keep it single then since that's what I've been doing in the rest of the code.

norraxx

ok, i've got it. 80 per line is not what we want, you have muuuch wider monotors. have at least 120 chars per row.

also, please fix sql query conditions

server/gameconnection.py

server/matchmaker/algorithm.py

server/matchmaker/matchmaker_queue.py

Askaholic · 2019-12-20T04:50:49Z

@norraxx this formatting comes from black. I’m not sure I really like it either.

Also since I’m the person who spends the most time working on this code base at the moment, I feel that it is fully within reason for me to require a max line length of 80 because my screen is not as big as yours, and I prefer not to squint or constantly scroll to read full lines when I’m coding.

norraxx · 2019-12-20T08:19:52Z

well, at my job, we are up to migrate to black... we do have code changes, so we are in same situation now, and we're not so happy :-)))
80 width is rough restriction from 30 years ago

eforgacs · 2019-12-27T20:05:16Z

Hey guys, sorry about the mess. I'm thinking about undoing this commit entirely and starting over (without using black). What are your thoughts?

norraxx · 2019-12-28T01:43:42Z

If you want, make git reset <hash of first non yours commit>, then git commit and git push force

eforgacs · 2019-12-28T03:07:36Z

If you want, make git reset <hash of first non yours commit>, then git commit and git push force

Yes, that's actually what @Askaholic recommended as well. I'll give that a shot.

* General code cleanup Removing unnecessary parentheses, test for membership should be "not in", typo fixes, indentation/spacing, changing from use of deprecated functions to non-deprecated functions, capitalization, fixing syntax of Bash scripts, line length, added "await" to coroutine that is not awaited. Changes in this commit: - Making TestClass use CamelCase (tests/unit_tests/test_decorators.py) * Single indenting run_lobby_server. * Fixing SQLAlchemy query to use the correct or condition (or_) * Changing docstrings to be comments.

norraxx reviewed Dec 2, 2019

View reviewed changes

Askaholic reviewed Dec 2, 2019

View reviewed changes

norraxx reviewed Dec 2, 2019

View reviewed changes

Askaholic reviewed Dec 14, 2019

View reviewed changes

norraxx reviewed Dec 19, 2019

View reviewed changes

eforgacs added 4 commits December 28, 2019 18:37

Single indenting run_lobby_server.

21a5ed2

Fixing SQLAlchemy query to use the correct or condition (or_)

8a870fa

Changing docstrings to be comments.

0fb7276

Merge branch 'develop' into develop

3b01996

Askaholic merged commit ed78458 into FAForever:develop Jan 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

General code cleanup #507

General code cleanup #507

eforgacs commented Nov 29, 2019

norraxx Dec 2, 2019

eforgacs Dec 2, 2019

Askaholic left a comment

Askaholic Dec 2, 2019

eforgacs Dec 2, 2019

Askaholic Dec 2, 2019

eforgacs commented Dec 2, 2019

norraxx Dec 2, 2019

Askaholic Dec 18, 2019

norraxx Dec 2, 2019

eforgacs Dec 15, 2019

Askaholic Dec 18, 2019

norraxx commented Dec 2, 2019

Askaholic Dec 14, 2019

eforgacs Dec 15, 2019

Askaholic Dec 16, 2019

norraxx left a comment

Askaholic commented Dec 20, 2019

norraxx commented Dec 20, 2019

eforgacs commented Dec 27, 2019

norraxx commented Dec 28, 2019 •

edited

Loading

eforgacs commented Dec 28, 2019

General code cleanup #507

General code cleanup #507

Conversation

eforgacs commented Nov 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Askaholic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eforgacs commented Dec 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norraxx commented Dec 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norraxx left a comment

Choose a reason for hiding this comment

Askaholic commented Dec 20, 2019

norraxx commented Dec 20, 2019

eforgacs commented Dec 27, 2019

norraxx commented Dec 28, 2019 • edited Loading

eforgacs commented Dec 28, 2019

norraxx commented Dec 28, 2019 •

edited

Loading