Add a tiny figlet font parser #47
Conversation
|
|
| override def aspects = List(TestAspect.jvmOnly) | ||
|
|
||
| override def spec = suite("parsing")( | ||
| testM("parse standard.flf") { |
There was a problem hiding this comment.
This is good. Maybe it would be nice to add a test that is normally @@ ignored, and which downloads all fonts from the website and tests that all of them can be parsed?
There was a problem hiding this comment.
Thinking of a resource like this:
http://www.figlet.org/fontdb.cgi
Wouldn't be too hard to just ZIO.foreach on a list of these files, scala.io.Source load them, and then parse them. Could catch possible errors and be run manually from time to time if anyone works in the parser code.
There was a problem hiding this comment.
Right, i had something like this in mind, and few related questions for zio-test: 1) to check @@ ignored 2) in my practice i found useful for such tasks to generate a report for visual inspection, hence how to better do it in zio-test, e.g. to gen a temp file, and log a URL in console -- does zio-test have a test-scoped log channel for that?
- Add manual integration test to utilize existing fonts, e.g. from http://www.figlet.org/fontdb.cgi
There was a problem hiding this comment.
ZIO Test doesn't have report generation yet, but will soon.
| object FigFontParser { | ||
| import ParseLib._ | ||
|
|
||
| def parse(lines: Seq[String]): Either[String, FigFont] = |
There was a problem hiding this comment.
Maybe we could move this to FigFont, as a constructor:
object FigFont {
...
def fromFile(content: String): Either[String, FigFont] = ???
def fromLines(content: Iterable[String]): Either[String, FigFont] = ???
...
}There was a problem hiding this comment.
- add
object FigFontwith publish API
| chars: Map[Char, FigChar] | ||
| ) | ||
|
|
||
| final case class FigChar(lines: Seq[FigCharLine], width: Int, height: Int) |
There was a problem hiding this comment.
Rather than use the type Seq, we should probably use zio.Chunk for high-performance and immutability.
There was a problem hiding this comment.
Initially, i was using Chunk everywhere there is Seq now, but for some reason was getting NoSuchMethodException in Chunk.head -- i wonder if the issue was caused by the parser being in the shared project while the tests in the jvm project -- I'll try to restore Chunk.
There was a problem hiding this comment.
Try chunk(0) instead of chunk.head (although keep in mind it is unsafe).
| @@ -0,0 +1,336 @@ | |||
| package zio.cli | |||
|
|
|||
| import scala.collection.mutable | |||
There was a problem hiding this comment.
Is a mutable collection necessary somewhere here?
There was a problem hiding this comment.
Just in one place: Parser#rep, i'll try to replace with FP using zio.Chunk -- it just coming to Scala all these allocations to support abstractions... need more time to calibrate the perception 🦀 😉
There was a problem hiding this comment.
It's fine as it is, I think, just wanted to check.
| ) | ||
| } | ||
|
|
||
| private[cli] object ParseLib { |
There was a problem hiding this comment.
Was this code imported or adapted from anywhere, such that we should reproduce copyright notices and ensure license compatibility? Or is it novel?
There was a problem hiding this comment.
Novel, but i looked at https://github.com/scala/scala-parser-combinators for inspiration. I'd like to revisit it again, as the font file parser turned out simpler than i anticipated 😄
|
Thanks @jdegoes! Having a lot of fun...😄😏 I'll continue working on the PR for a few days, and ping you for a final review. I wish I was better familiar with zio codebases and FD from the start, so my ad hoc code was more conforming -- i'd like to refactor to better match zio-codec, but probably keep the executable encoding 😉 |
|
@domartynov Sounds great, just let me know when you're ready for that final review! |
|
Be sure also to format the code using |
|
@jdegoes i've updated the PR:
|
|
@domartynov Amazing work! Thank you for your contribution and let me know if you like the t-shirt. 😆 💪 |
|
@jdegoes Thanks! yes, sure, i think i've submitted the form during the hackathon 😉 |
wip