Scanner option to expand variations within a single game

[mikeb26]

Hi,

I am a former contributor to https://github.com/notnil/chess and just recently discovered that you've taken over the library. I am working on porting over the 4 outstanding pull requests that I had sent. 3 of the 4 were trivial and I've posted those PRs. The 4th is non-trivial due to what looks like a heavy refactor of scanner.go. For reference my original PR is here: notnil/chess#127 which added a scanner option to expand variations within a single game. e.g. For a .pgn containing:

[Event "opening prep"]
[Result "*"]
[Variant "Standard"]
[ECO "C10"]
[Opening "French Defense: Paulsen Variation"]
[UTCDate "2025.07.10"]
[UTCTime "19:53:02"]

1. e4 e6 2. d4 d5 3. Nc3 { Paulsen French } (3. e5 { Advance French }) (3. exd5 { Exchange French }) *

Scanner today will treat that as a single game. My PR added a flag which allow the consumers of Scanner to specify that Scanner() should instead return 3 games. Before proceeding with rewriting the feature I wanted some feedback first. My thought was to extend Scanner.nextGame into a slice (https://github.com/CorentinGS/chess/blob/main/scanner.go#L75) and then have ScanGame() (https://github.com/CorentinGS/chess/blob/main/scanner.go#L102) check for the option. when the option is set, re-parse s.scanner.Text() and split into multiple GameScanned. In this way the only performance penalty for the default Scanner is the test for the option; otherwise all of the performance tax is borne by users who want to expand variations. There are certainly alternative approaches though and thought I should solicit feedback before proceeding.

[CorentinGS]

Thanks for the detailed proposal, @mikeb26 ! This looks like a great addition to the library, and I appreciate you taking the time to contribute.

I'm happy with the suggested approach. Please go ahead with the implementation.

Before you submit the pull request, could you please make sure that all the existing tests are passing? Also, it would be great if you could add a few new tests to cover the new "expand variations" functionality.

Finally, please add a note to the documentation regarding this change. If there are any potential breaking changes, please detail them there.

Looking forward to seeing the PR!

[mikeb26]

Before you submit the pull request, could you please make
sure that all the existing tests are passing? Also, it would
be great if you could add a few new tests to cover the
new "expand variations" functionality.

Yes; original PR added tests to cover the new functionality and existing tests were all passing. I'll make sure the same is true when porting the PR here.

Finally, please add a note to the documentation regarding
this change.

Will do; I missed this in my original PR.

If there are any potential breaking changes, please detail them there.

The original change was not breaking and I do not expect this one to be either. Existing NewScanner() callers will not need to change and will continue to get the same functionality as it exists today. My change can either add a new optional parameter to NewScanner() (preferred) or a separate NewScannerWithOptions(). No other changes to the public API are needed.

[mikeb26]

@CorentinGS posted the PR here: #56 After a more careful study of the changes you've made from the original notnil/chess.git I missed these 2 key observations when making my original proposal:

1. You've changed the public API exposed by the Scanner to be more lower level than the original scanner. That's great for callers who want fine grained control over the intermediate parsing states, but complicated how I originally intended to approach the new expandvar scanner option.
2. You've augmented the Game parser to store the tree of parsed variations within the Game instance. That's a nice upgrade from the original, and I really wanted to leverage this work you've already done here vs. having a separate parser for variations which is what I had done in Scanner: Add option flag to expand variations notnil/chess#127.

So after considering these instead of converting Scanner.nextGame into []GameScaned, I introduced a separate and distinct nextParsedGames []*Game. In order to leverage the work you had already done on parsing variations I introduced Game.Split(). Finally, I added a separate and distinct Scanner.ParseNext() as a higher level scanner primitive that's more similar to the original notnil/chess.git for the most common use case of just iterating Game * from a pgn file. I left the lower level iterator primitives in place so that callers have the option of using either.

Feedback welcome if you'd prefer a different approach.

[CorentinGS]

I like your approach, it seems good to me. I'll merge the PR. Thank you very much for contributing to the project, it helps a lot. If you have any suggestion or advice for the project, feel free to open a new issue or a discussion, I'd love to discuss more with you on how to improve the library and make it better for the users.

[mikeb26]

will do. closing this one for now.