Skip to content

upgrade pangram tests to 2.0.0#871

Merged
sshine merged 2 commits intoexercism:masterfrom
chiroptical:pangram_sync
Oct 25, 2019
Merged

upgrade pangram tests to 2.0.0#871
sshine merged 2 commits intoexercism:masterfrom
chiroptical:pangram_sync

Conversation

@chiroptical
Copy link
Copy Markdown
Contributor

This PR updates the pangram tests original mentioned in #852

I found this commit exercism/problem-specifications@f68b632 describing the recent changes to the tests. It seems that the descriptions are the only thing that changed. Is the goal to match the descriptions? Or match the canonical tests described in the specifications?

@petertseng
Copy link
Copy Markdown
Member

petertseng commented Oct 15, 2019

The thing that is most important is the content. As we can see from git show f68b632f2578d9969074657d764b32f61f53c008 -w, only descriptions and indentation were changed in 2.0.0. I would say that unless a particular description doesn't make sense for the language that we might as well try to match the descriptions as well.

The original motivation for the changed descriptions was that descriptions that are too long don't work well for some tracks. This track does not appear to have such a problem, so if there are some description changes that actually lose some information, we can keep them.

On the other hand if there are some changes whose purpose is to be better English, I would say to probably apply them to this track as well. "sentence empty" vs "empty sentence" would be in that category, I suppose?

@chiroptical
Copy link
Copy Markdown
Contributor Author

Great, I'll go through and compare the descriptions to see if any make sense to incorporate.

@chiroptical
Copy link
Copy Markdown
Contributor Author

chiroptical commented Oct 18, 2019

In my previous PR #843 @sshine helped me with the it function from hspec. I modified the descriptions such that they read "pangram ...", e.g. "pangram with empty sentence". I think this helps the readability. Let me know what you think.

Copy link
Copy Markdown
Contributor

@sshine sshine left a comment

Choose a reason for hiding this comment

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

@barrymoo: The main difference between #843 and here is that the phrases there are for property-based tests that are not shared with the problem-specifications repo, and the attempt here is to somewhat sync the unit tests. We may have an interest on the Haskell track in slightly deviating from canonical tests when the purpose of shortening a test description does not apply.

Although we use Hspec, we generally don't think much about how test output renders in practice. Perhaps this is because canonical tests don't assume a "readable spec" format with "it" being the initial combinator. Both choices have their pros and cons: With this PR we get better output, but further syncing becomes slightly harder, since the difference must be analysed and the reasoning is found in this PR / commit and not in the test suite itself.

I'm willing to accept these changes on the grounds that the problem-specifications repo is temporarily out of order (exercism/problem-specifications#1560) and will most likely work differently when we get something back that fills its role. In the meantime, we should continue to improve on the Haskell track.

I will wait 48 hours for other maintainers to interject before squash merging this.

@sshine sshine merged commit 8dd9f2b into exercism:master Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants