Skip to content

Conversation

Chazzz
Copy link
Contributor

@Chazzz Chazzz commented Dec 11, 2020

For the record, ResourceRequestEvent and ResourceRequestFulfillEvent are also not integrated.

@Talv
Copy link
Contributor

Talv commented Dec 11, 2020

Also make sure to run black on it, so it'll pass the tests: https://github.com/ggtracker/sc2reader/blob/upstream/CONTRIBUTING.md#coding-style

@cclauss
Copy link
Collaborator

cclauss commented Dec 11, 2020

#!/bin/bash -eo pipefail
black . --check

would reformat /home/circleci/project/sc2reader/events/game.py
would reformat /home/circleci/project/test_replays/test_replays.py
Oh no! 💥 💔 💥
2 files would be reformatted, 45 files would be left unchanged.

@Chazzz
Copy link
Contributor Author

Chazzz commented Dec 11, 2020

@Talv @cclauss Resolved formatting issues.

Copy link
Contributor

@Talv Talv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

self.minerals, self.vespene, self.terrazine, self.custom_resource
return (
self._str_prefix()
+ " requests {0} minerals, {1} gas, {2} terrazine, and {3} custom".format(
Copy link
Collaborator

@cclauss cclauss Dec 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The {0} {1} {2} {3} thing has not been needed since Python 2.6 unless you want substitutions to land out of order or you want substitutions to repeat.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how it was originally written and how the other classes in this file are written as well. Maybe create a separate issue to change this more broadly?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me

@StoicLoofah StoicLoofah merged commit 3694fc9 into ggtracker:upstream Dec 11, 2020
@StoicLoofah
Copy link
Collaborator

Great fixes and thanks to other for reviewing this!

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.

4 participants