Skip to content
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

feat: implement figurine algebraic notation #578

Conversation

KartikWatts
Copy link
Member

@KartikWatts KartikWatts commented Aug 6, 2024

closes #487

This PR implements FAN notation.

@KartikWatts KartikWatts marked this pull request as draft August 6, 2024 11:30
Copy link
Member Author

@KartikWatts KartikWatts left a comment

Choose a reason for hiding this comment

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

@programarivm Would like to know your thoughts, will complete the development for this PR based on the knowledge. Regards.


use Chess\Variant\Classical\PGN\Move;

class FanMovetext extends SanMovetext
Copy link
Member Author

Choose a reason for hiding this comment

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

After some thought, I concluded it would be better to extend from SanMovetext as most of the logic could be reused.

Copy link
Member

@programarivm programarivm Aug 9, 2024

Choose a reason for hiding this comment

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

@KartikWatts, Chess\Movetext\FanMovetext can definitely be extended from Chess\Movetext\SanMovetext.

<?php

namespace Chess\Movetext;

class FanMovetext extends SanMovetext
{
    public function validate(): string
    {
        $validated = parent::validate();

        return $this->toFan($validated);
    }

    public function filtered($comments = true, $nags = true): string
    {
        $filtered = parent::filtered($comments, $nags);

        return $this->toFan($filtered);
    }

    protected function toFan(string $movetext)
    {
        // TODO
    }
}

Keep it up,

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi, after some thought, I realized that both $validated and $movetext are strings, so for cases where comments and nags are also included, simple conversion as in Js-utils won't work, but rather we have to traverse through the string with explode(' ', ... to convert only the pieces and not the whole string in toFan. I didn't find it optimum to traverse two times separately, for validate and filtered methods.

So, conversely, I found it better to have the implementation to convert the properties in proper format straight-way during insert, so that we won't be required to convert them separately later.
Instead, I maintained $sanMoves differently which can be utilized for validation purposes.

Though If we decide it better to call toFan two times rather, let me know, in that case we can traverse through the array: foreach (explode(' ', $this->validated) as $key => $val) { in toFan method, instead of insert as currently done.

Comment on lines +52 to +56
$movetext = '1.d4 Nf6 2.Nf3 e6 3.c4 Bb4+ 4.Nbd2 O-O 5.a3 Be7 6.e4 d6 7.Bd3 c5';

$expected = [ 'd4', '♘f6', '♘f3', 'e6', 'c4', '♗b4+', '♘bd2', 'O-O', 'a3', '♗e7', 'e4', 'd6', '♗d3', 'c5' ];

$this->assertEquals($expected, (new FanMovetext(self::$move, $movetext))->moves);
Copy link
Member Author

Choose a reason for hiding this comment

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

My most important question as discussed in the issue discussion as well was to understand, if this is the correct test case that satisfies the requirement, i.e. for standard notation provided the moves would be in FAN, if this is true I'll extend on to it to write more test cases based on the understanding.

Copy link
Member

Choose a reason for hiding this comment

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

Probably the best thing to do is to extend from SanMovetext as described above.

Replaced R, N and B with miniature piece icons.
@programarivm
Copy link
Member

programarivm commented Aug 9, 2024

@KartikWatts FanMovetextTest has been updated with some changes. At this moment Q and K need to be replaced with miniature piece icons as well. Then, FanMovetext should be rewritten for the tests in FanMovetextTest to pass.

I hope this helps.

Keep it up,

Revert the changes back
@programarivm
Copy link
Member

Also the changes in SanMovetext have been reverted back since this one should not be changed in order to implement FanMovetext.

Let's recap, this the way to proceed now:

  1. Q and K need to be replaced with miniature piece icons in FanMovetextTest
  2. Rewrite FanMovetext for the tests in FanMovetextTest to pass

Keep it up and happy learning!

Comment on lines +62 to +81
public function get_san_movetext()
{
$movetext = '1.d4 Nf6 2.Nf3 e6 3.c4 Bb4+ 4.Nbd2 O-O 5.a3 Be7 6.e4 d6 7.Bd3 c5';

$expected = [ 'd4', '♘f6', '♘f3', 'e6', 'c4', '♗b4+', '♘bd2', 'O-O', 'a3', '♗e7', 'e4', 'd6', '♗d3', 'c5' ];

$this->assertEquals($expected, (new FanMovetext(self::$move, $movetext))->moves);
}

/**
* @test
*/
public function get_san_moves()
{
$movetext = '1.d4 ♘f6 2.♘f3 e6 3.c4 ♗b4+ 4.♘bd2 O-O 5.a3 ♗e7 6.e4 d6 7.♗d3 c5';

$expected = [ 'd4', 'Nf6', 'Nf3', 'e6', 'c4', 'Bb4+', 'Nbd2', 'O-O', 'a3', 'Be7', 'e4', 'd6', 'Bd3', 'c5' ];

$this->assertEquals($expected, (new FanMovetext(self::$move, $movetext))->sanMoves);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The fact that we have toFan method in our FanMoveText class, it only make more sense if the class has ability to receive SAN text as well in the constructor itself.
Likewise, having the ability to get SAN moves is nice.
So, I have implemented 2 tests for the scenarios, let me know your thoughts on this. If approved I can write more tests for other possible cases. Else I can remove the tests.

@KartikWatts KartikWatts marked this pull request as ready for review August 16, 2024 08:35
@programarivm programarivm merged commit 85eea7e into chesslablab:main Aug 16, 2024
3 checks passed
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 close these issues.

Implement the Figurine Algebraic Notation
2 participants