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

Feat: add mmr for Players and Observers #190

Merged
merged 3 commits into from
Sep 1, 2023

Conversation

NumberPigeon
Copy link
Contributor

should solve issue #89

@@ -285,6 +285,21 @@ def __init__(self, uid, init_data):
#: This is deprecated because it doesn't actually work.
self.recorder = None

#: The user's mmr at the time of the game
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is mmr and why does the reader need to guess?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MMR stands for Matchmaking Rating, indicating the skill level on SC2 ladder. I assume this is a common abbreviation like APM under sc2 context, but now I dont know whether we should explain it in comment or not.

sc2reader/objects.py Outdated Show resolved Hide resolved
Co-authored-by: Christian Clauss <cclauss@me.com>
sc2reader/objects.py Outdated Show resolved Hide resolved
Co-authored-by: Christian Clauss <cclauss@me.com>
Copy link
Collaborator

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@StoicLoofah StoicLoofah left a comment

Choose a reason for hiding this comment

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

thanks for the detailed comment!

@StoicLoofah StoicLoofah merged commit a68dc10 into ggtracker:upstream Sep 1, 2023
2 checks passed
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.

3 participants