queen-attack: represent positions as objects#935
Conversation
|
Nice change! Please also bump the canonical data version as appropriate: https://github.com/exercism/problem-specifications#test-data-versioning :) |
|
@stkent Thanks for the reminder, totally forgot about that! Since we're changing the type of one of the data keys I assume this is a major bump, correct? |
|
That would be my interpretation of the guidelines, yes! |
|
Thanks, bumped major number. |
Insti
changed the title
| "position": "(2,2)" | ||
| "position": { | ||
| "file": 2, | ||
| "rank": 2 |
There was a problem hiding this comment.
The description uses the terms row and column I think we should stick with those.
There was a problem hiding this comment.
Agree with @Insti, we should follow what is in the description (or else change the description).
There was a problem hiding this comment.
I'd argue against changing the description. (But it is certainly an option.)
There was a problem hiding this comment.
I agree, row/column works great for me. Made the change.
NobbZ
left a comment
NobbZ
left a comment
There was a problem hiding this comment.
As with #936, I do not think that the proposed changes justify a major bump.
1.0.1 should be bumped enough, since only representation of testdata changed, which each language is free to choose anyway and no one is forced to change anything in the corresponding exercises of their tracks.
|
It seems as if I had to revoke my request for changes, as per the version numbering documentation, we have to bump MAJOR when we break test generators, and this changes propably will:
I previously always understood the version as a version of the exercise, to understand evolvement of exercises, but it seems as if it were only meant to check for compatibility in testgenerators, nothing more, nothing less. |
There was a problem hiding this comment.
I am glad it is getting done.
No opinion on rank/file vs row/column, but I will remark that I often forget which is which and have to remind myself that we talk about the a-file, b-file, etc., so there is an advantage to row/column even if it is not specific to the domain
Squash on merge.
(Don't worry, I'm not merging until we deal with rank/file vs row/column, this approval just means I ultimately would take either)
|
I'm totally fine with row/column, let's use that. Just submitted the patch. Anything else? |
There was a problem hiding this comment.
require 'json'
require_relative '../../verify'
json = JSON.parse(File.read(File.join(__dir__, 'canonical-data.json')))
raise "There should be exactly two cases, not #{json['cases'].size}" if json['cases'].size != 2
verify(json['cases'][0]['cases'], property: 'create') { |c|
c['queen']['position'].values_at('row', 'column').all? { |x| (0...8).cover?(x) } ? 0 : -1
}
verify(json['cases'][1]['cases'], property: 'canAttack') { |c|
w, b = %w(white black).map { |color|
c["#{color}_queen"]['position'].values_at('row', 'column')
}
diffs = w.zip(b).map { |wb| wb.reduce(:-).abs }
diffs.include?(0) || diffs.uniq.size == 1
}says it is good.
Squash on merge.
petertseng
left a comment
petertseng
left a comment
There was a problem hiding this comment.
these two don't need to be fixed in this PR.
| "expected": 0 | ||
| }, | ||
| { | ||
| "description": "queen must have positive rank", |
There was a problem hiding this comment.
this is very awkward that the description (and thus the keys in the JSON file) use "row" yet the description uses "rank". We'll want to fix that up later (or now??)
There was a problem hiding this comment.
I'm willing to change it now.
| "column": 2 | ||
| } | ||
| }, | ||
| "expected": 0 |
There was a problem hiding this comment.
a little awkward that these are 0/-1 instead of just true/false. Probably worth fixing later on.
There was a problem hiding this comment.
Right now it's using 0/1 as false/true and -1 as error. It would probably be better to use the accepted "error" methodology, but that's probably worth its own pull request. Should one of us open an issue?
There was a problem hiding this comment.
I agree that a separate PR for this is a good idea.
|
@petertseng I made the changes in the test case descriptions like you mentioned. |
queen-attack: use rank/file instead of x and y queen-attack: bump major version for queen-attack queen-attack: Use row/column instead of rank/file in test keys queen-attack: use row/column instead of rank/file in json descriptions
HarrisonMc555
force-pushed
the
queen-attack-position-encoding
branch
from
3577218 to
330c197
Compare
6 participants
Closes #721.
I decided to use objects, since it seems like most people were agreeing it was more explicit. I will also be using objects to encode the position for #722, which is deals with a similar issue.