Skip to content

Make queen-attack clearer for non-chess players #1965

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

Closed
wants to merge 3 commits into from

Conversation

mahmoud-moursy
Copy link

  • Add an explanation for what B and W are (the queens)
  • Add examples of queens seeing each other, and not interacting.

- Add an explanation for what B and W are (the queens)
- Add examples of queens seeing each other, and not interacting.
@SleeplessByte
Copy link
Member

cc @exercism/reviewers

Copy link
Contributor

@SaschaMann SaschaMann left a comment

Choose a reason for hiding this comment

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

I don't personally see the value of adding info to the description that is, as you say yourself, not relevant information to the puzzle, but if we decide to add it, it should be done as an advanced admonition imo. It meets the description for that kind of admonition perfectly:

Information that is only relevant for people who want to dig more deeply into something or are expected to have more advanced knowledge.

@@ -8,6 +8,7 @@ A chessboard can be represented by an 8 by 8 array.

So if you are told the white queen is at `c5` (zero-indexed at column 2, row 3) and the black queen at `f2` (zero-indexed at column 5, row 6), then you know that the set-up is like so:

###### B and W are the queens, standing for Black and White. This is not relevant info to the puzzle.
Copy link
Contributor

@SaschaMann SaschaMann Feb 21, 2022

Choose a reason for hiding this comment

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

This would make more sense as an admonition or at least normal text. It doesn't act as a heading in this document.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this should not be a heading. I think having the content here is a good idea by @T-O-R-U-S , but I would wrap it in:

~~~~exercism/note
B and W are the queens, standing for Black and White.
This is not relevant info to the puzzle.
~~~~

Copy link
Member

Choose a reason for hiding this comment

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

@T-O-R-U-S I see that you already added the proposed note, but also make sure to remove this heading.

Comment on lines 55 to 58
B and W stand for **Black** and **White**, the two sides competing
against each other in a game of chess, which is why each queen
is labelled B and W; though you do not need to know which side
the queens are on to solve this challenge.
Copy link
Contributor

Choose a reason for hiding this comment

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

The Style Guide requires one sentence per line.

@mahmoud-moursy
Copy link
Author

I don't personally see the value of adding info to the description that is, as you say yourself, not relevant information to the puzzle, but if we decide to add it, it should be done as an advanced admonition imo. It meets the description for that kind of admonition perfectly:

Information that is only relevant for people who want to dig more deeply into something or are expected to have more advanced knowledge.

I believe that it has value though, as B and W may be confusing to people trying to solve the puzzle. Perhaps B and W should just be changed to Q, standing for queen? Thought this might break some prior solutions to the problem.

@@ -8,6 +8,7 @@ A chessboard can be represented by an 8 by 8 array.

So if you are told the white queen is at `c5` (zero-indexed at column 2, row 3) and the black queen at `f2` (zero-indexed at column 5, row 6), then you know that the set-up is like so:

###### B and W are the queens, standing for Black and White. This is not relevant info to the puzzle.
Copy link
Member

Choose a reason for hiding this comment

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

Yes, this should not be a heading. I think having the content here is a good idea by @T-O-R-U-S , but I would wrap it in:

~~~~exercism/note
B and W are the queens, standing for Black and White.
This is not relevant info to the puzzle.
~~~~

_ _ _ _ _ _ B _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ B _ _ _ _
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
```
Copy link
Member

Choose a reason for hiding this comment

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

This could use an explanation of what you're seeing.

Copy link
Member

Choose a reason for hiding this comment

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

An example of queens attacking already exists in the description. I am not convinced that we need more. The textual description In the game of chess, a queen can attack pieces which are on the same row, column, or diagonal. sounds clear enough to me.

However, if we do want more examples, then:

  • They should all use the same notation, the existing ones have labels "a b c..." and "1 2 3...".
  • They should be grouped together, not scattered in two different places.
  • +1 to @SleeplessByte that each example should come with an explanation.

Copy link
Member

Choose a reason for hiding this comment

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

I'm also of the opinion that these examples are not needed. I do like the explaining of B and W.

_ _ _ _ _ _ B _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
_ _ _ _ _ _ _ _ _ _ B _ _ _ _ _ _ _ _ _ _ _ B _
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
```
Copy link
Member

Choose a reason for hiding this comment

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

This could use an explanation of what you're seeing.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, we could use Q for both, since it is explained that the side does not matter, the black nor the white makes a difference.

Copy link
Member

@angelikatyborska angelikatyborska left a comment

Choose a reason for hiding this comment

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

I think adding a note explaining "B" and "W" is a nice idea, especially for non-native speakers.

PS: Make sure to fix all of the linting issues.

@@ -8,6 +8,7 @@ A chessboard can be represented by an 8 by 8 array.

So if you are told the white queen is at `c5` (zero-indexed at column 2, row 3) and the black queen at `f2` (zero-indexed at column 5, row 6), then you know that the set-up is like so:

###### B and W are the queens, standing for Black and White. This is not relevant info to the puzzle.
Copy link
Member

Choose a reason for hiding this comment

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

@T-O-R-U-S I see that you already added the proposed note, but also make sure to remove this heading.

_ _ _ _ _ _ B _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ B _ _ _ _
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
```
Copy link
Member

Choose a reason for hiding this comment

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

An example of queens attacking already exists in the description. I am not convinced that we need more. The textual description In the game of chess, a queen can attack pieces which are on the same row, column, or diagonal. sounds clear enough to me.

However, if we do want more examples, then:

  • They should all use the same notation, the existing ones have labels "a b c..." and "1 2 3...".
  • They should be grouped together, not scattered in two different places.
  • +1 to @SleeplessByte that each example should come with an explanation.

Comment on lines +12 to +21
a b c d e f g h a b c d e f g h a b c d e f g h
8 _ _ _ _ _ _ _ _ 8 8 _ _ _ _ _ _ _ _ 1 8 _ _ _ _ _ _ _ _ 8
7 _ _ _ _ _ _ _ _ 7 7 _ _ _ _ _ _ _ _ 2 7 _ _ _ W _ _ _ _ 7
6 _ _ _ W _ _ _ _ 6 6 _ B _ _ _ W _ _ 3 6 _ _ _ _ _ _ _ _ 6
5 _ _ _ _ _ _ _ _ 5 5 _ _ _ _ _ _ _ _ 4 5 _ _ _ _ _ _ _ _ 5
4 _ _ _ _ _ _ _ _ 4 4 _ _ _ _ _ _ _ _ 5 4 _ _ _ _ _ _ _ _ 4
3 _ _ _ _ _ _ B _ 3 3 _ _ _ _ _ _ _ _ 6 3 _ _ _ _ _ _ _ _ 3
2 _ _ _ _ _ _ _ _ 2 2 _ _ _ _ _ _ _ _ 7 2 _ _ _ B _ _ _ _ 2
1 _ _ _ _ _ _ _ _ 1 1 _ _ _ _ _ _ _ _ 8 1 _ _ _ _ _ _ _ _ 1
a b c d e f g h a b c d e f g h a b c d e f g h
Copy link
Member

Choose a reason for hiding this comment

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

@T-O-R-U-S You have modified the first example of queens attacking so that it no longer matches the text before it. Not only the positions are different than in the text (the example shows d6 and g3, not c5 and f2 as the text says), but the text is clearly written for a single example, not for 3. Maybe we can just revert this part to the original and only leave the three new examples of queens not attacking?

Additionally, the CI is complaining about linting problems. Those need to be fixed before we can approve the PR.

@safwansamsudeen
Copy link
Contributor

@T-O-R-U-S are you still willing to work on this or can I take it up?

@IsaacG
Copy link
Member

IsaacG commented Apr 29, 2023

There's been multiple comments calling out that the docs are pretty clear and that this change may not add much value. Unless anyone feels there's a need to clarify things, I propose leaving well enough alone and closing this PR. Or, rather, given how long this PR has been open, I'll just close it out. Feel free to reopen if you think the problem would benefit from clarifications.

@IsaacG IsaacG closed this Apr 29, 2023
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.

8 participants