-
-
Notifications
You must be signed in to change notification settings - Fork 554
queen-attack: should represent positions as array or object, not string #721
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
Comments
I think that using strings is definitely not a good way to represent tuples in JSON. Number should always be represented as numbers. 👍 for an array or an object. |
Great point! I think having an object would be best as it is the most explicit. |
Nice catch, @petertseng! In fact, the conclusion from the PR was to specifically not use AN. Do we have a process for recording exercise-specific "policy"s? |
Ah, (other than using the policy label, if we should so choose) we had the idea of the philosophy document in #382 - no exercise has yet had one. |
Closes #721. I decided to use objects, since it seems like most people were agreeing it was more explicit. Since the description uses row/column instead of rank/file, the objects do so as well, and this means the test cases descriptions are also changed to use those terms.
Currently, positions are represented as strings like
"(2, 2)"
in the canonical data for queen-attack.For languages where
(2, 2)
is valid syntax, this seems fine - the test generator will simply be able to interpolate this string wherever it is necessary.For those languages where it is not valid syntax, it seems like an unusual burden to force test generators for that language to parse the rank and file from the string
"(2, 2)"
.Consider changing this to an array containing two integers such as
[2, 2]
or an object with explicit keys such as{"rank": 2, "file": 2}
.Note that we don't currently decide to use true algebraic notation for these coordinates: #446, so both the rank and file should remain as numbers.
The text was updated successfully, but these errors were encountered: