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

robot-simulator: encode position as object instead of string #936

Merged

Conversation

HarrisonMc555
Copy link
Contributor

Closes #722.

I decided to use objects. Some people preferred arrays and some people preferred objects, but it seems like in other problems objects were preferred because they were more explicit (see #721). I don't believe we have a policy yet on whether to use arrays or objects for things like this, but I believe that objects would be a better decision. There are some situations in which it doesn't really matter and some people would prefer arrays, but there are some situations where it really would be better to have objects. Since it sometimes is desirable to have objects and only sometimes slightly preferred to have arrays, I think objects are a better default policy.

@stkent
Copy link
Contributor

stkent commented Oct 9, 2017

Nice change! Please also bump the canonical data version as appropriate: https://github.com/exercism/problem-specifications#test-data-versioning :)

@HarrisonMc555
Copy link
Contributor Author

Thanks, bumped major number.

Copy link
Contributor

@Insti Insti left a comment

Choose a reason for hiding this comment

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

Good improvement.

NobbZ
NobbZ previously requested changes Oct 9, 2017
Copy link
Member

@NobbZ NobbZ left a comment

Choose a reason for hiding this comment

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

I do not think, that we really need a major bump.

Nothing has changed. All implementations of the tests can decide to keep the old data structure internally and map the new canonical representation to the most idiomatic representation of the language as it is done with so many other JSON objects already…

So version should in my opinion only get bumped to 1.0.1.

@NobbZ
Copy link
Member

NobbZ commented Oct 9, 2017

Since I can't alter my review… Of course I like the overal change, since it is much more clear how the coordinates have to be read.

Also I do clearly prefer objects over arrays/lists here for the representation of the testdata, since {"x": 1, "y": 2} is so much more obvoius than [1, 2]. To be honest, there were not much of a semantic difference between [1, 2] ans "(1, 2)"

@Insti
Copy link
Contributor

Insti commented Oct 9, 2017

So version should in my opinion only get bumped to 1.0.1.

I think I agree with this.

(I'm not sure why you cannot 👍 reviews.)

@NobbZ
Copy link
Member

NobbZ commented Oct 9, 2017

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:

MAJOR changes should be expected to break even well-behaved test generators.

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.

@NobbZ NobbZ dismissed their stale review October 9, 2017 20:57

Misunderstanding versioning guidelines

@Insti
Copy link
Contributor

Insti commented Oct 9, 2017

@NobbZ do you want to make an issue for this discussion? I think it's worth having but we seem to be having this discussion across 3 different PRs. It would be good to have it all in one place we can tag as policy at some point.

@NobbZ
Copy link
Member

NobbZ commented Oct 9, 2017

Yeah, I was going to do so, but first I had to remove false negatives to not block the process of merging.

@NobbZ NobbZ mentioned this pull request Oct 9, 2017
Copy link
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

I am glad it is getting done.

Squash on merge.

Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

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

Love it! This will allow me to simplify my test generator :)

@petertseng petertseng merged commit 80a4b5f into exercism:master Oct 10, 2017
@HarrisonMc555
Copy link
Contributor Author

Hey sorry I missed the party. I think I was supposed to squash but didn't get around to it in time. Thanks everyone!

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.

6 participants