8000 Fix ambiguous moves in PGN; Add PGN test case by TheNipanen · Pull Request #47 · CorentinGS/chess · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix ambiguous moves in PGN; Add PGN test case #47

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

TheNipanen
Copy link
@TheNipanen TheNipanen commented Jun 22, 2025

Ambiguous moves were being generated in game PGNs as per #44. The reason was that the generator was looking at the predecessor position when determining what information was needed for the current move. This PR fixes the issue.

EDIT: The predecessor position was necessary for processing subvariations in the PGN encode. This was brough back in a more robust way, so that ambiguous moves were fixed while keeping the predecessor position for the subvariations. Also added a test case that captures the example from #44

@CorentinGS CorentinGS requested a review from Copilot June 23, 2025 12:58
Copilot

This comment was marked as outdated.

@TheNipanen TheNipanen changed the title Fix ambiguous moves in PGN Fix ambiguous moves in PGN; Add PGN test case Jun 28, 2025
@TheNipanen TheNipanen requested a review from Copilot June 30, 2025 12:46
Copy link
@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes the issue of ambiguous moves generated in PGNs by updating the move encoding logic to conditionally use the predecessor position for subvariations while preserving legacy behavior for standard moves. It also adds a new test case to capture the ambiguous move scenario reported in #44.

  • Fix ambiguous move generation in PGN encoding.
  • Update move encoding to use the predecessor position only in subvariation mode.
  • Add a test case to verify the correct PGN output for longer games.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
game_test.go Added a test case ("GameStringWithLongerGame") that verifies PGN output for a longer game sequence.
game.go Modified the writeMoveEncoding function to incorporate a 'subVariation' flag for disambiguation.
Comments suppressed due to low confidence (2)

game.go:494

  • [nitpick] Consider renaming the parameter 'subVariation' to 'isSubVariation' for improved clarity regarding its boolean nature.
func writeMoveEncoding(node *Move, currentMove *Move, subVariation bool, sb *strings.Builder) {

game.go:495

  • Add a comment to explain why the parent's position is used when subVariation is true to enhance clarity on the PGN disambiguation logic.
	if subVariation && node.Parent() != nil {

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 clo 369A se these issues.

1 participant
0