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

secret-handshake: Add canonical data (revives #159) #489

Merged
merged 2 commits into from Jan 15, 2017
Merged

secret-handshake: Add canonical data (revives #159) #489

merged 2 commits into from Jan 15, 2017

Conversation

petertseng
Copy link
Member

@petertseng petertseng commented Jan 8, 2017

The time has come to help the abandoned #159 get merged.

Implementing tracks:

Summary of tests among these languages:

  • All language tracks test: 1, 2, 4, 8, 3, 19, 31.
  • Go, Haskell, Objective C, Scala, and Swift additionally test 0.
  • Go is the only track that tests 32, 33.
  • Python is the only track that tests 9, 22, 5, -9.
  • Python is the only track that tests action -> number conversions.
  • Haskell, Python, and Scala test strings (string binary -> number conversion)
  • ECMAScript and JavaScript test strings (only that strings are rejected)
  • Perl and Ruby test strings (that they are accepted but result in an empty array)

Closes #159 by way of replacement.
Closes exercism/todo#145

"expected": ["jump"]
},
{
"description": "reversing no actions still gives the no actions",
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 no?

@petertseng petertseng changed the title secret-handshake: Add canonical data (reapply #159) secret-handshake: Add canonical data (replaces #159) Jan 8, 2017
@petertseng
Copy link
Member Author

I have to not use the term "reapply" since I have in the past used it to mean "revert a revert commit", so I don't want to overload the term.

@petertseng petertseng changed the title secret-handshake: Add canonical data (replaces #159) secret-handshake: Add canonical data (revives #159) Jan 9, 2017
Copy link
Contributor

@IanWhitney IanWhitney left a comment

Choose a reason for hiding this comment

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

The description file mentions 9 as an example. Where possible I like to include a test for any examples included in the description file.

Also, the description file still mentions the binary-string conversion, which you have removed. So we should remove that from the description file.

@Insti
Copy link
Contributor

Insti commented Jan 14, 2017

Even though I've approved the current version, I also agree with @IanWhitney 's points.

@petertseng
Copy link
Member Author

petertseng commented Jan 14, 2017

I've a decision to make.

Situation:

  • All tracks test 3 and 19.
  • README has 9 and "11001" (25)

I could do one of two three things:

  • Make README use 3 and 19
  • Add 9 and 25 to the test file, in addition to 3 and 19 (but then what does one pair test that the other doesn't?)
  • Add 9 and 25 to the test file, replacing 3 and 19 (implies churn in tracks that wish to keep exactly up to date with the test file)

I think it's best to make the README use 3 and 19. Coming right up.

abeger and others added 2 commits January 14, 2017 11:54
Implementing tracks:

* https://github.com/exercism/xcsharp/blob/master/exercises/secret-handshake/SecretHandshakeTest.cs
* https://github.com/exercism/xecmascript/blob/master/exercises/secret-handshake/secret-handshake.spec.js
* https://github.com/exercism/xfsharp/blob/master/exercises/secret-handshake/SecretHandshakeTest.fs
* https://github.com/exercism/xgo/blob/master/exercises/secret-handshake/secret_handshake_test.go
* https://github.com/exercism/xhaskell/blob/master/exercises/secret-handshake/test/Tests.hs
* https://github.com/exercism/xjavascript/blob/master/exercises/secret-handshake/secret-handshake.spec.js
* https://github.com/exercism/xlua/blob/master/exercises/secret-handshake/secret-handshake_spec.lua
* https://github.com/exercism/xobjective-c/blob/master/exercises/secret-handshake/SecretHandshakeTest.m
* https://github.com/exercism/xperl5/blob/master/exercises/secret-handshake/handshake.t
* https://github.com/exercism/xpython/blob/master/exercises/secret-handshake/handshake_test.py
* https://github.com/exercism/xruby/blob/master/exercises/secret-handshake/secret_handshake_test.rb
* https://github.com/exercism/xscala/blob/master/exercises/secret-handshake/src/test/scala/SecretHandshakeTest.scala
* https://github.com/exercism/xswift/blob/master/exercises/secret-handshake/SecretHandshakeTest.swift

Summary of tests among these languages:

* All language tracks test: 1, 2, 4, 8, 3, 19, 31.
* Go, Haskell, Objective C, Scala, and Swift additionally test 0.
* Go is the only track that tests 32, 33.
* Python is the only track that tests 9, 22, 5, -9.
* Python is the only track that tests action -> number conversions.
* Haskell, Python, and Scala test strings (string binary -> number conversion)
* ECMAScript and JavaScript test strings (only that strings are rejected)
* Perl and Ruby test strings (that they are accepted but result in an empty array)

Closes #159 by way of replacement.
Closes https://github.com/exercism/todo/issues/145

Changes since #159:

* remove decode and composition cases
  Discussion in #159 indicates that the decode step should be removed.
  Since composition includes decode, that implies composition should be
  removed as well.
* Test only integers
  Discussion in #159 indicates we don't do binary conversions, so all
  string inputs are geeting removed.
* Test only positive integers
  It's recommended to use unsigned integers for bitwise operations.
* describe importance of each test
* All actions is 15 (1111), not 16 (10000)
* Add cases for reverse singleton and nothing
We'd like to use 3 and 19 to match up with what's in the test cases
already used by most languages. We also do not want to do binary to
integer conversions, by discussion in #159.

This covers most tasks in #335, though I would probably still leave #335
open because explicit discussion of numbers larger than 11111 has not
been added.
@petertseng
Copy link
Member Author

The commits for the canonical-data.json have been squashed down to one, and the README change added as a separate commit. I think the JSON change and the README change should be kept as separate commits? I don't know, I could be convinced either way on that one.

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.

I like the separate commits.

@petertseng petertseng merged commit ca0431c into exercism:master Jan 15, 2017
@petertseng petertseng deleted the secret-handshake-json branch January 15, 2017 00:08
emcoding pushed a commit that referenced this pull request Nov 19, 2018
Add the bookkeeping module to accumulate_test.rb
Add bookkeeping to example.rb
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.

None yet

4 participants