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

fixed parsing of scenario matches #100

Closed
wants to merge 1 commit into from

Conversation

Skazu
Copy link
Contributor

@Skazu Skazu commented Jul 7, 2023

This PR enables the parsing of games with a selected user scenario, such as this one: https://www.aoe2insights.com/match/245096119/.
Without the changes, an exception occurs when parsing the scenario data.
I was not able to check a possible downward compatibility, perhaps the structure has changed here with a special DE version?

@Skazu Skazu marked this pull request as draft July 7, 2023 08:24
@Skazu Skazu force-pushed the master branch 3 times, most recently from 2a0ee9a to 6ecfff1 Compare July 7, 2023 09:08
@Skazu
Copy link
Contributor Author

Skazu commented Jul 7, 2023

I am confident that the current status is compatible now.
I have added a new test and a rec containing a user scenario.
Only the test that tries all available recs in the tests/recs folder with the full header parser fails now, but as far as I understood, the full header parser is no longer be maintained, so maybe this test should be removed or narrowed to not just test all recs?

@Skazu Skazu marked this pull request as ready for review July 7, 2023 12:01
@happyleavesaoc
Copy link
Owner

The full header parsing is still maintained and should continue to function (you're probably thinking of the full body parsing). Looks like this change should be easy to add to the full parser, though.

Here's the relevant section: https://github.com/happyleavesaoc/aoc-mgz/blob/master/mgz/header/scenario.py#L28

That block should be moved into messages.

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