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

Fix #134 - add some string and regex ops #135

Merged
merged 1 commit into from
Apr 11, 2020

Conversation

erikerlandson
Copy link
Contributor

Adds several String methods and Regex methods

@erikerlandson
Copy link
Contributor Author

This isn't complete (needs unit tests) but thought I'd push it so folks can take a look.

Copy link
Collaborator

@soronpo soronpo left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!
Other than the tests, I mentioned only minor issues.

@codecov
Copy link

codecov bot commented Apr 10, 2020

Codecov Report

Merging #135 into master will decrease coverage by 1.65%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #135      +/-   ##
==========================================
- Coverage   90.93%   89.28%   -1.66%     
==========================================
  Files          10       10              
  Lines         662      681      +19     
  Branches       11       15       +4     
==========================================
+ Hits          602      608       +6     
- Misses         60       73      +13     
Impacted Files Coverage Δ
src/main/scala/singleton/ops/package.scala 100.00% <ø> (ø)
.../main/scala/singleton/ops/impl/GeneralMacros.scala 88.62% <100.00%> (-1.88%) ⬇️

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 14131af...840cdc1. Read the comment docs.

@erikerlandson
Copy link
Contributor Author

note to self, also add new ops to readme, and look at twoface, Substring shows up there

@soronpo
Copy link
Collaborator

soronpo commented Apr 10, 2020

and look at twoface, Substring shows up there

Oh, right. I forgot about that. TwoFace.String should get the extra operations.
You can leave that to me, since I'm guessing I'm the only one currently using TwoFace, it's not that big of deal if the added operation will be added to it as well later on.

@soronpo
Copy link
Collaborator

soronpo commented Apr 10, 2020

Lets change the RegExZZZZ operations from being a applied to a regex, to their equivalent application to a string.
E.g.,
Instead of RegExMatches[R, S] we just have Matches[S, RegEx], which is equivalent to the runtime String.matches(regex).
This will be consistent with all other operations we currently have.

@soronpo
Copy link
Collaborator

soronpo commented Apr 10, 2020

For the tests, it's enough to cover:

  • A literal result for each operation
  • A non-literal result for each operation (no need for all literal/non-literal input combinations)
  • A bad input (to trigger the unsupported() calls) for each operation
  • Where there is option for error/exception, add a relevant test.

@erikerlandson
Copy link
Contributor Author

erikerlandson commented Apr 10, 2020

A non-literal result for each operation

I had a couple questions about this.

  1. I notice that some operations, like Length, only do CalcVal and not CalcLit, and I was wondering why.
  2. How does one test CalcVal vs CalcLit? For example, CharAtSpec looks like LengthSpec, but CharAt has both modes.

@soronpo
Copy link
Collaborator

soronpo commented Apr 10, 2020

A non-literal result for each operation

I had a couple questions about this.

1. I notice that some operations, like `Length`, only do `CalcVal` and not `CalcLit`, and I was wondering why.

2. How does one test CalcVal vs CalcLit? For example, `CharAtSpec` looks like `LengthSpec`, but CharAt has both modes.

You are right. It was a mistake that these functions were left out from refactoring back in the day when I did the trick with a CalcVal constructor. Substring should actually look like this

def Substring : Calc = (a, b) match {
  case (CalcVal(at : String, att), CalcVal(bt : Int, btt)) => CalcVal(at.substring(bt), q"$att.substring($btt)")
  case _ => unsupported()
}

The CalcVal(literal,tree) decides either to construct a CalcLit or CalcNLit from an implicit that is set at https://github.com/fthomas/singleton-ops/blob/master/src/main/scala/singleton/ops/impl/GeneralMacros.scala#L894

@soronpo
Copy link
Collaborator

soronpo commented Apr 10, 2020

  1. How does one test CalcVal vs CalcLit? For example, CharAtSpec looks like LengthSpec, but CharAt has both modes.

As for the runtime, leave it be. I'm checking it through TwoFace, so I'll add the checks there when I add it.

@erikerlandson
Copy link
Contributor Author

@soronpo the latest changes caused three tests in TwoFaceStringSpec.scala to fail compile. It wasn't obvious to me why, so I commented them out while I worked on unit testing the string ops.

@erikerlandson
Copy link
Contributor Author

Also, the codacy troll is demanding payment but I'm using style that also appears elsewhere, so not sure what to make of that.

@soronpo
Copy link
Collaborator

soronpo commented Apr 11, 2020

@soronpo the latest changes caused three tests in TwoFaceStringSpec.scala to fail compile. It wasn't obvious to me why, so I commented them out while I worked on unit testing the string ops.

I understand why. Rebase on top #136 and use CalcVal.mayFail for your operations that may fail.

@soronpo
Copy link
Collaborator

soronpo commented Apr 11, 2020

Also, the codacy troll is demanding payment but I'm using style that also appears elsewhere, so not sure what to make of that.

Ignore the codacy troll. I might do more refactoring down the line and I will address those issues.

@erikerlandson
Copy link
Contributor Author

Oh wait, mayFail is taking by-name value : => Any and doing the try/catch for me.

@erikerlandson
Copy link
Contributor Author

@soronpo I folded in #136 and put back the twoface tests

@soronpo soronpo merged commit 32ff823 into fthomas:master Apr 11, 2020
@soronpo
Copy link
Collaborator

soronpo commented Apr 11, 2020

Thanks again!

@erikerlandson
Copy link
Contributor Author

Nuts, I think this breaks on scala 2.12

[error] /home/eje/git/singleton-ops/src/main/scala/singleton/ops/impl/GeneralMacros.scala:1288:49: value matches is not a member of scala.util.matching.Regex
[error]         CalcVal.mayFail(Primitive.Boolean, bt.r.matches(at), q"$btt.r.matches($att)")

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.

2 participants