Skip to content

Conversation

Gusgus01
Copy link
Contributor

Anonymized replays are missing the main replay.initData and
replay.details files, this will fallback to the backup versions.
Add a new print statement case to the GameEvent base to cover
when just the player.name is missing.

Used a replay called out in #61 to test.

@Gusgus01 Gusgus01 force-pushed the AnonymizedReplayWorkaround branch from f3c2df8 to 48df941 Compare November 25, 2018 07:14
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.

I think this looks awesome! Thanks for digging in and figuring out a solution for this.

Just a few minor comments to cleanup the code. Overall, though, the approach looks awesome!

@Gusgus01
Copy link
Contributor Author

Thanks for the comments! Really helps to have someone look over the changes.

Anonymized replays are missing the main replay.initData and
replay.details files, this will fallback to the backup versions.
Add a new print statement case to the GameEvent base to cover
when just the player.name is missing.
@Gusgus01 Gusgus01 force-pushed the AnonymizedReplayWorkaround branch from 48df941 to 3aa603c Compare November 26, 2018 13:46
@Gusgus01
Copy link
Contributor Author

Gusgus01 commented Nov 26, 2018

I agreed with all your requested changes and I think I made them all. (Whoops, I force-pushed to my branch to maintain the single commit and that makes it not quite possible to view the old code outside of the preview windows above)

I attempted to capture the logic in a test (test_game_event_string), then added the new case. Assuming I captured the logic correctly, it looks like I made the changes correctly.

I also thought about using a replay and an anonymized replay, but I couldn't find a "Global" example in my replays, so I went with the mock route to hit each branch of logic to make sure it stayed the same.

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.

looks good! You put a lot more work into that unit test than I would have, but I'm not going to complain about tests =)

@StoicLoofah StoicLoofah merged commit e58e8e5 into ggtracker:upstream Nov 28, 2018
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.

2 participants