Bob's conversational partner is a purist! #713
Conversation
There was a problem hiding this comment.
Thank you. You should make this change in https://github.com/exercism/problem-specifications/blob/master/exercises/bob/canonical-data.json#L165-L172 , as we wouldn't want to be selfish in keeping this change to ourselves.
| @@ -1,5 +1,6 @@ | |||
| module Bob (responseFor) where | |||
| import Data.Char (isSpace, isUpper, isAlpha) | |||
| import Data.Char (isSpace, isUpper, isAlpha, isPunctuation) | |||
| import Safe | |||
There was a problem hiding this comment.
don't forget to add safe to exercises/bob/examples/success-standard/package.yaml.
|
Thank you; I made #1319 on exercism/problem-specifications as you suggested. Will the tests for all language tracks be rebuilt based on this, or will the language track maintainers be notified of the change somehow? |
|
Also, should I squash these commits before the PR is accepted? I experienced on the Ocaml language track that reviewing became difficult when I rebased during review. |
It's good to ask, since different reviewers have different preferences. I can't speak for any other reviewers, but here are mine: It is not necessary to avoid rebasing. If I would be bothered by a rebase, I will check out the changes locally so I can diff against the last version I reviewed. It is usually not necessary to squash, since it is possible that I will do it myself, whether via the interface that GitHub provides to do so, or manually. If doing it myself will be too cumbersome and I think some particular arrangement of commits makes the most sense, I will ask about it and request that it happen then. |
|
Whoops, I'm sorry to have leaked an unrelated commit into this PR. I'm still getting used to GitHub. |
|
Indeed, you will want to remove that commit from this PR, because each PR should only be focused on a single task. You can do so by removing it from the branch that this PR is based off of, which is the branch listed after "from" in the portion of the page that reads "merge 4 commits into Don't hesitate to ask if you need help figuring out how to do so. |
|
I removed the rogue commit and squashed the remaining ones related to this issue. (That sounded violent.) |
There was a problem hiding this comment.
Good stuff. So let me let you know of a few bookkeeping things: Since this is problem-specifications 1.4.0, I'd like https://github.com/exercism/haskell/blob/master/exercises/bob/package.yaml#L2 changed.
However, that file also indicates Haskell track needs to get up to date with the change in problem-specifications 1.3.0 (which would be exercism/problem-specifications#1293 )
So there are three choices to proceed:
- Combine the test changes into this PR, having one PR move
bobfrom 1.2.0 to 1.4.0 - Put 1.2.0 to 1.3.0 in a separate PR and this PR only deals with 1.3.0 to 1.4.0
- Do nothing and make me do the 1.2.0 to 1.3.0 change, and this PR will only deal with 1.3.0 to 1.4.0. This is an option because I wouldn't want to hold up 1.4.0 and make you do more work just because this track is not up to date.
I have no particular preference between these three options right now.
|
I'll go with option 2 tomorrow. |
In the Bob exercise it says:
> Bob's conversational partner is a purist when it comes to written
> communication and always follows normal rules regarding sentence
> punctuation in English.
But in the unit test the following input is fed to `responseFor`:
"\nDoes this cryogenic chamber make me look fat?\nno"
This is a rhetorical question, and by the expected value, this is deemed
to not be an actual question, which is very reasonable. But if Bob's
conversational partner is a punctuation purist, it should at least be
"no." with the period.
I found this by wanting to grab the last character that `isPunctuation`,
but had to go with `not . isSpace` (like the reference solution). But
this really sounds wrong when I think about it: I actually want the last
punctuation character and am allowed to assume "normal rules".
Making this change will not render `not . isSpace` solutions wrong.
Bump test version to 1.4.0.7.
|
@petertseng I think if you merge #717 and then this one, versions should add up. |
Otherwise, a student may assume DMV isn't shouted. Prerequisite to #713. Addresses exercism/problem-specifications#1293.
That is true, though neither Git nor GitHub are smart enough to understand what to do with the two commits changing the same line. Nevertheless, easy enough to tell it what we want, and it was to be expected anyway. (As in the last PR, don't panic about my adding a merge commit in, repository requirements, it will be squashed) |
|
Finally, you had a question.
To date, there has not been a good system for this, neither automatic rebuilds nor automatic notifications. The relevant project is exercism/meta#99 Until it is completed, language track maintainers are kind of on their own. Some might use scripts similar to https://github.com/petertseng/exercism-problem-specifications/tree/up-to-date/up-to-date to compare their track's versions vs that of problem-specifications. There may be other solutions to this problem floating around out there. In certain cases, a concerned individual decides the change is so important that it warrants opening an issue in all tracks. Certainly nothing preventing such a concerned individual from doing so, and there is even https://github.com/exercism/blazon that assists in such a matter. Whether it actually gets done is usually decided case-by-case depending on how concerned the individual is. |
@petertseng: Do you have a preference between these options today? In that case, I'd like to clarify the formulation in #852. |
|
When I consider my preference today, I keep in mind that my preference may affect how a contributor is able to contribute to this repository. Therefore: no preference. A contributor may decide to only apply the earlier of two version changes, and leave the later one to a different contributor. Note that I didn't mention what should happen when a contributor only wants to apply the later of two version changes but leave the earlier one for a different contributor. But that raises difficulties on what, if anything, the version should be changed to. But I don't really expect anyone to want to do that. In all cases, the relevant problem-specifications PRs must be linked. But that is already a stipulation of #852. |
In the Bob exercise it says:
But in the unit test the following input is fed to
responseFor:This is a rhetorical question, and by the expected value, this is deemed
to not be an actual question, which is very reasonable. But if Bob's
conversational partner is a punctuation purist, it should at least be
"no." with the period.
I found this by wanting to grab the last character that
isPunctuation,but had to go with
not . isSpace(like the reference solution). Butthis really sounds wrong when I think about it: I actually want the last
punctuation character and am allowed to assume "normal rules".
Making this change will not render
not . isSpacesolutions wrong.