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

Use game_type instead of init_mode in game_launch message #685

Closed
Askaholic opened this issue Oct 17, 2020 · 12 comments · Fixed by #857
Closed

Use game_type instead of init_mode in game_launch message #685

Askaholic opened this issue Oct 17, 2020 · 12 comments · Fixed by #857

Comments

@Askaholic
Copy link
Collaborator

Init mode can be inferred from the game type i.e. matchmaker -> auto lobby, otherwise -> normal lobby.

We should add a game_type field to the game_launch message just like how it is in the game_info message and add a comment stating that init_mode is deprecated.

"game_type": GameType.to_string(self.game_type),

"init_mode": game.init_mode.value,

@angelobzsouza
Copy link

Could I try to help on it?

@Askaholic
Copy link
Collaborator Author

Yea sure! It should be pretty straight forward:

  1. Add the game_type field, and comment that init_mode is deprecated.
  2. Add one or more unit tests asserting the presence of the field.
  3. (Optional) refactor the GameType enum to use string values instead of integers. You can use one of the other enums as an example, like InitMode, or Faction to name a few.

Some tips from past experience with new contributors:

  1. If you’re using Windows, then it’s probably not worth trying to get everything setup locally, just make a PR and use the Travis build to view the output of unit tests. This is because setting up Docker for the database is a pain on Windows (apparently one person was able to get it running under WSL).
  2. If you have any questions, just ask! I check my email for github notifications pretty regularly.

@wrharding
Copy link

I would like to help on this issue. I'll be leaning on Travis while I ensure I've worked out all the quirks with WSL & Docker for Desktop.

@Askaholic
Copy link
Collaborator Author

Alright, no problem. I usually just amend and force push if I need to use the CI instead of local tests. But you can also add a bunch of commits and squash/rebase them later when you've worked everything out. Doesn't matter a whole lot since we "squash and merge" PR's anyways.

@Askaholic
Copy link
Collaborator Author

@wrharding any progress on this? Do you need help getting everything set up?

@wrharding
Copy link

Hello! Yes I am still working on this. My setup has the ability to create the infrastructure needed to get the tests running, I'm just taking it rather slow to get familiar. I've made the code changes, but haven't got the stack + db up and running yet for testing. If you would like, I can just use the Travis output to fix any issues that might come about. That way the issue doesn't look stale.

@wrharding
Copy link

@Askaholic for whatever reason these two unit tests are expecting the data returned in a different order. All the data is there, but the test fails because they're expecting 'uid' before 'mod' (and now don't include the 'game_type' attribute):
test_lobbyconnection.py::test_command_game_join_calls_join_game
test_lobbyconnection.py::test_command_game_join_uid_as_str

Would you prefer these be resolved in a separate PR or can I just bundle them all into one?

@Askaholic
Copy link
Collaborator Author

Alright no worries, I just wasn't sure if you were still working on it (as you can maybe see, the last guy who worked on it got about halfway through and then disappeared :P). If you need some tips on setting up the db on windows let me know (you can also message me through the FAF discord).

The order of attributes in a dictionary doesn't matter for equality. You just need to add the game_type field to the expected call.

>>> {1: "foo", 2: "bar"} == {2: "bar", 1: "foo"}
True

@KaukaHan
Copy link
Contributor

KaukaHan commented Nov 11, 2021

i've setup all the infrastructure to run and test the dev server. As far as i can see, game_type has already been put in.
So whats left is 1) a comment that init_mode is obsolete or can be even removed ?
2) some unittests
and 3) Refactor game_type to use strings

@KaukaHan
Copy link
Contributor

about 3): the mentioned initMode and Faction Enums dont use string either. I can work on that too

@KaukaHan
Copy link
Contributor

oh i didnt noticed somone is still working on this. @wrharding are you still working on this ?

@wrharding
Copy link

I was working on it for hacktoberfest 2020 but never completed it. Feel free take it and complete the three things you outlined.

KaukaHan added a commit to KaukaHan/server that referenced this issue Nov 14, 2021
Askaholic pushed a commit that referenced this issue Nov 17, 2021
* add a game_type field to the game_launch message

* added missing game_type field in test

* add game_type to launch_game message

* fix typo

* change GameType member types to string

* use lower case

* to_string is not used anymore for GameType

* do not inherit from str

* remove inheritage from str

* remove unused import

* remove unused import

* remove unused import

* add missing import

* import formatting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment