python-chess icon indicating copy to clipboard operation
python-chess copied to clipboard

Handle multiple comments on a move/variation/game

Open MarkZH opened this issue 1 year ago • 10 comments

Some chess programs that write PGN files (including lichess.org and others mentioned in the linked issue) will write multiple consecutive braced comments--e.g., 1. e4 { A very common opening } { Perhaps too common ... } { [%clk 0:00:01] }. Previously, when read by chess.pgn.read_game(), these comments would be joined into a single comment with a single space between them. This PR creates a new class to read, store, and write these comments while keeping them separate. The user interface is kept backwards compatible by using @property decorators to allow assignment and comparison of bare strings and string lists to game node comments.

Closes #946

MarkZH avatar Feb 23 '24 07:02 MarkZH

Hi. Good idea.

I think changing the property type in this way is not quite backwards compatible, even if __str__ is provided. Maybe something like

class GameNode:

    comments: List[Comment]

    @property
    def comment(self) -> str: ...

    @comment.setter
    def comment(self, value: str) -> None: ...

could work?

Or maybe it's also just time to consider 2.x at some point.

niklasf avatar Apr 18 '24 15:04 niklasf

I tried to make GameNodeComment backwards compatible in terms of assignment and writing to a PGN file. The breaking change comes from getting comments from a node (with some allowances for comparing a single comment to a single string), since I didn't want users to accidentally miss comments. What would GameNode.comment return, just the first comment?

Saving this until 2.x would make things much easier. A new class wouldn't even be needed.

class GameNode:
    comments: list[str]

If you want to save this PR until 2.x, this would be an easy change.

MarkZH avatar Apr 20 '24 04:04 MarkZH

In 1.x, GameNode.comment would have to be the concatenation of all comments, to match the current behavior.

niklasf avatar Apr 20 '24 08:04 niklasf

That should do it. GameNode.comments returns the GameNodeComment (a list[str] essentially), and GameNode.comment returns a single str formed from joining the strings with spaces.

MarkZH avatar Apr 21 '24 04:04 MarkZH

Do you want to save this functionality for v2.0? I can make a new PR (not backwards compatible) that would be much simpler than this one--basically replacing all comment: str members of GameNode and ChildNode with comments: list[str] and changing the PGN comment functions.

MarkZH avatar May 16 '24 05:05 MarkZH

Let's do 2.x. So it would contain "small" breaking changes and clean-ups, and more ambitious plans and far-reaching changes would be pushed back to 3.x.

niklasf avatar May 25 '24 15:05 niklasf

Updated changes:

  1. Rename GameNode comment field from comment to comments and starting_comment to starting_comments.
  2. Adapt methods that process comments (e.g., eval() and set_eval()).
  3. Adapt PGN reading and writing to handle moves/games with multiple comments.

The change from comment to comments is the breaking change. Assigning to node.comment no longer has any effect. I figured that using a plural name emphasizes that the field should be a list. Plus, assigning a string comment instead of a list of strings will cause problems with comment processing methods (Item 2, above).

MarkZH avatar May 26 '24 22:05 MarkZH

Looks like the release of mypy 1.11 is catching new errors.

MarkZH avatar Jul 31 '24 07:07 MarkZH

Fixed via #1100. Targetting one final 1.x release before making the jump to 2.x.

niklasf avatar Jul 31 '24 16:07 niklasf

Should extra whitespace be preserved when reading comments? To ask another way, what was the reason for this commit? I would think that a newline in the middle of a curly brace comment was put there by word wrapping and so would not be a significant part of the comment. Is there a problem with formatting imported comments with the equivalent of

comment = " ".join(comment.split())

to replace all whitespace with single spaces?

MarkZH avatar Aug 03 '24 09:08 MarkZH