Code review - sgf parsing library

I've been working for a while on this library: julianandrews/sgf-parse to parse (and serialize) SGF files (a format used primarily for records of Go games, but which supports a number of different strategy games).

I've recently finished a large scale rewrite to follow the spec more accurately instead of making a bunch of Go specific assumptions, and I'm hoping to get pointers and feedback. On the one hand, I'm pretty proud of where I've gotten it, but on the other hand, I've been working mostly in a vacuum, and don't know if I've produced a sensible library, or some sort of outsider art, so any advice is welcome!

1 Like

Link correction (I assume): GitHub - julianandrews/sgf-parse: SGF parsing library for Rust.

Whoops! Yup. Corrected in the comment. I got that really mangled somehow!

1 Like

Hi Julian,

I took a quick look your API seems well-thought-out but I also have absolutely no clue of SGF, so take that with a grain of salt. Glancing over the code, I didn't notice anything to improve. I like that you linked the relevant specification sections in your documentation.

What I do notice API-wise is that parse() returns a vector, maybe that could be changed to an iterator? But I don't know if incremental parsing is something you might want to do with SGF files.

Thanks for the feedback! If nothing else, it's good to have some indication I'm not just working on my own outsider-art!

I wen't back and forth on parse() returning a vector vs. an iterator. I settled on the vector because currently my parsing isn't incremental. That said, I could change that in the future - and maybe returning an iterator would better future proof the interface. I suppose there's not much down side!

A standard SGF file for a single game is <5k in size, and the largest file I've ever seen was about 1.2M, so I'm not sure there's too much to gain from incremental parsing. On the other hand, SGFs can in principle hold an arbitrary number of games, so I could imagine someone doing AI work wanting to generate millions of games.

I think I'll go ahead and make that change for v5 =)

You're welcome =)

Yeah, returning an iterator implies that the parser works incrementally, so I'd probably keep at as is.
On a more practical note, you might want to add some position to SgfParseError to indicate on which line & column the error occurred.

Something I think is quite cool are Cargo examples. If you have e.g. examples/checksgf.rs, you can run it with cargo run --example checksgf myfile.sgf.

Serde support would probably also be desirable to e.g. convert SGF files to other formats like JSON, though I don't know how much effort that would be.

you might want to add some position to SgfParseError to indicate on which line & column the error occurred.

I had totally meant to do that, and then I never got around to it! I should go back and do that!

you might want to add some position to SgfParseError to indicate on which line & column the error occurred.

That's a fantastic idea. I'll be doing that for sure!

Serde support would probably also be desirable to e.g. convert SGF files to other formats like JSON, though I don't know how much effort that would be.

I should look into that - though I doubt there's much call for converting SGFs to other formats.

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.