hohav / peppi

Rust parser for Slippi SSBM replay files
MIT License
37 stars 9 forks source link

Update parser to support through slippi version 3.12.0 #5

Closed NickCondron closed 2 years ago

NickCondron commented 2 years ago

Adds support for 3.10, 3.11, 3.12, and fixes parsing issues with 3.9 (closing #4)

Tested working on local replay version 3.12.

PS: the regex bug in peppi-derive was terrible to track down lol

Potential todos:

hohav commented 2 years ago

Thanks for the PR; looks great so far. I'm super busy so I may not get a chance to review for a while, but I haven't forgotten about this.

NickCondron commented 2 years ago

Thanks a bunch for the contribution, and sorry for taking so long to review it. I've left a few comments. Besides those, we should also add tests for the new fields, and for the netplay code fix. Let me know if you don't have time to finish things up, and I can take over (but I may not get around to it for a while, either...).

No problem. Thanks for writing Peppi. I'm using it for one of my projects https://github.com/NickCondron/slp-search (not yet finished), and the performance is amazing. I can write the tests, but might not get around to it until this weekend.

Do you have any feedback about working on peppi, by the way? Were there particular areas that were hard to understand, or would've benefited from more documentation?

Yes. I'm new to Rust, so most of my problems came from unfamiliarity with the language/tools. However there were a few things.

From a Peppi development perspective, I didn't have too many issues. I was kind of able to copy how other things were done before. I suppose something that explained how the slippi(version) attribute works would be useful.

From a Peppi user perspective:

If I think of more things I will add them here. If you think some of these are good ideas. I can create an issue and work on them down the line.