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

Reduce redundancy in Semigroup and Eq test names #2081

Merged
merged 1 commit into from
Dec 8, 2017

Conversation

ceedubs
Copy link
Contributor

@ceedubs ceedubs commented Dec 8, 2017

Previously if you ran the CommutativeSemigroup tests on a type (Mean
in this example) you would see something like:

MeanTests:
- Mean.commutativeSemigroup.commutative
- Mean.commutativeSemigroup.semigroup associative
- Mean.commutativeSemigroup.semigroup combineAllOption
- Mean.commutativeSemigroup.semigroup repeat1
- Mean.commutativeSemigroup.semigroup repeat2

The extra semigroup in the name seems redundant to me, and as far as I
can tell, it's inconsistent with tests for other type classes. Eq had
a similar issue.

Previously if you ran the `CommutativeSemigroup` tests on a type (`Mean`
in this example) you would see something like:

```
MeanTests:
- Mean.commutativeSemigroup.commutative
- Mean.commutativeSemigroup.semigroup associative
- Mean.commutativeSemigroup.semigroup combineAllOption
- Mean.commutativeSemigroup.semigroup repeat1
- Mean.commutativeSemigroup.semigroup repeat2
```

The extra `semigroup` in the name seems redundant to me, and as far as I
can tell, it's inconsistent with tests for other type classes. `Eq` had
a similar issue.
Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

You're on fire! Thanks for this :)

@codecov-io
Copy link

codecov-io commented Dec 8, 2017

Codecov Report

Merging #2081 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2081      +/-   ##
==========================================
- Coverage   94.66%   94.63%   -0.04%     
==========================================
  Files         318      318              
  Lines        5383     5383              
  Branches      207      207              
==========================================
- Hits         5096     5094       -2     
- Misses        287      289       +2
Impacted Files Coverage Δ
...in/scala/cats/kernel/laws/discipline/EqTests.scala 100% <100%> (ø) ⬆️
...a/cats/kernel/laws/discipline/SemigroupTests.scala 100% <100%> (ø) ⬆️
...main/scala/cats/kernel/laws/PartialOrderLaws.scala 90.9% <0%> (-9.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7bb92b2...5699360. Read the comment docs.

Copy link
Contributor

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

Thanks!

@kailuowang kailuowang merged commit 31349a9 into typelevel:master Dec 8, 2017
@kailuowang kailuowang added this to the 1.0.0 milestone Dec 8, 2017
@ceedubs ceedubs deleted the test-names branch December 8, 2017 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants