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

Replace TrimmedString regex with Trimmed predicate type #504

Merged
merged 3 commits into from
May 10, 2018

Conversation

ceedubs
Copy link
Contributor

@ceedubs ceedubs commented May 10, 2018

This also adds TrimmedString.trim and is a replacement for #487 based
on the discussion there.

While we are at it I threw in an Arbitrary instance for TrimedString.

This also adds `TrimmedString.trim` and is a replacement for fthomas#487 based
on the discussion there.
@@ -42,7 +42,7 @@
</check>
<check level="error" class="org.scalastyle.scalariform.NumberOfTypesChecker" enabled="true">
<parameters>
<parameter name="maxTypes"><![CDATA[40]]></parameter>
<parameter name="maxTypes"><![CDATA[45]]></parameter>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding Trimmed bumped us over the limit here. 41 seemed a bit too arbitrary so I bumped this to 45. I will not attempt to defend this solution.

Copy link
Owner

@fthomas fthomas left a comment

Choose a reason for hiding this comment

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

This looks great, thanks @ceedubs!

The build failed on 2.10 and 2.11 because MiMa complained about the added Arbitrary instance. We can add this in a binary compatible way via a new StringInstancesBinCompat1 trait - similar how new Arbitrary instances were added in https://github.com/fthomas/refined/pull/486/files

@codecov
Copy link

codecov bot commented May 10, 2018

Codecov Report

Merging #504 into master will increase coverage by 1.22%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #504      +/-   ##
==========================================
+ Coverage   93.15%   94.37%   +1.22%     
==========================================
  Files          61       62       +1     
  Lines         672      676       +4     
  Branches       10       14       +4     
==========================================
+ Hits          626      638      +12     
+ Misses         46       38       -8
Impacted Files Coverage Δ
...c/main/scala/eu/timepit/refined/types/string.scala 100% <100%> (ø) ⬆️
...n/scala/eu/timepit/refined/scalacheck/string.scala 100% <100%> (ø) ⬆️
...red/src/main/scala/eu/timepit/refined/string.scala 100% <100%> (ø) ⬆️
...in/scala-2.12/eu/timepit/refined/api/Refined.scala 0% <0%> (ø)
...c/main/scala/eu/timepit/refined/cats/package.scala 100% <0%> (+20%) ⬆️
...main/scala/eu/timepit/refined/scalaz/package.scala 100% <0%> (+72.72%) ⬆️

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 25a9e4e...b63d61f. Read the comment docs.

@ceedubs
Copy link
Contributor Author

ceedubs commented May 10, 2018

Thanks @fthomas. I've hopefully fixed the binary compatibility.

@fthomas
Copy link
Owner

fthomas commented May 10, 2018

Looks great, thanks again @ceedubs!

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

2 participants