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

add hardLine to force line breaks #181

Merged
merged 7 commits into from
Sep 12, 2019
Merged

add hardLine to force line breaks #181

merged 7 commits into from
Sep 12, 2019

Conversation

johnynek
Copy link
Collaborator

close #76

The inspiration here was to split #117 into two. The first part was #176 which we could add without relaxing laws. This is the second part which exposes the ability to make hardLine documents.

We only removed two laws:

  1. Line never appears other than on the left of FlatAlt. This is obvious since it is the feature we are adding.
  2. Union(a + b, a + c) == a + Union(b, c) is only true if a contains no hardLines.

We found a simpler, but very similarly performing implementation (sometimes faster sometimes a bit slower) of fill which was easier to reason about and allowed us to not have to give up that combinator. Here are the benchmarks. The x suffix means the previous implementation:

[info] Benchmark                 (size)   Mode  Cnt     Score     Error   Units    
[info] PaigesBenchmark.fill0          1  thrpt   10  6506.563 ± 189.848  ops/ms        
[info] PaigesBenchmark.fill0         10  thrpt   10   605.373 ±  17.366  ops/ms        
[info] PaigesBenchmark.fill0        100  thrpt   10    63.641 ±   0.662  ops/ms        
[info] PaigesBenchmark.fill0       1000  thrpt   10     6.344 ±   0.041  ops/ms        
[info] PaigesBenchmark.fill0      10000  thrpt   10     0.625 ±   0.007  ops/ms        
[info] PaigesBenchmark.fill100        1  thrpt   10  6530.974 ± 240.346  ops/ms        
[info] PaigesBenchmark.fill100       10  thrpt   10   631.062 ±  10.576  ops/ms        
[info] PaigesBenchmark.fill100      100  thrpt   10    47.490 ±   0.323  ops/ms        
[info] PaigesBenchmark.fill100     1000  thrpt   10     5.010 ±   0.036  ops/ms        
[info] PaigesBenchmark.fill100    10000  thrpt   10     0.535 ±   0.015  ops/ms        
[info] PaigesBenchmark.fillx0         1  thrpt   10  6041.646 ±  38.923  ops/ms        
[info] PaigesBenchmark.fillx0        10  thrpt   10   657.951 ±  43.749  ops/ms        
[info] PaigesBenchmark.fillx0       100  thrpt   10    63.629 ±  17.652  ops/ms        
[info] PaigesBenchmark.fillx0      1000  thrpt   10     7.019 ±   0.452  ops/ms        
[info] PaigesBenchmark.fillx0     10000  thrpt   10     0.709 ±   0.016  ops/ms        
[info] PaigesBenchmark.fillx100       1  thrpt   10  6029.988 ± 358.575  ops/ms                       
[info] PaigesBenchmark.fillx100      10  thrpt   10   623.129 ±   4.476  ops/ms                       
[info] PaigesBenchmark.fillx100     100  thrpt   10    44.471 ±   3.276  ops/ms    
[info] PaigesBenchmark.fillx100    1000  thrpt   10     5.174 ±   0.208  ops/ms    
[info] PaigesBenchmark.fillx100   10000  thrpt   10     0.559 ±   0.003  ops/ms

This does not make flatten and flattenOption private. Maybe we should. But it does change the contract: there is no guarantee there are no newlines, the result may have hardLine in them. It may be better to just make them private. I don't know if anyone really needs them.

This was work @non and I did together, but highly based on the excellent pathbreaking work of @seanmcl.

cc @olafurpg @seanmcl

@codecov-io
Copy link

codecov-io commented Sep 12, 2019

Codecov Report

Merging #181 into master will decrease coverage by 0.14%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #181      +/-   ##
=========================================
- Coverage   97.84%   97.7%   -0.15%     
=========================================
  Files           4       4              
  Lines         279     305      +26     
  Branches       27      38      +11     
=========================================
+ Hits          273     298      +25     
- Misses          6       7       +1
Impacted Files Coverage Δ
core/src/main/scala/org/typelevel/paiges/Doc.scala 98.23% <100%> (+0.22%) ⬆️
.../src/main/scala/org/typelevel/paiges/package.scala 0% <0%> (-100%) ⬇️

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 21f0d09...1174422. Read the comment docs.

Copy link
Contributor

@non non 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 good! I saw two minor issues which should be pretty easy to fix.

@@ -800,6 +867,23 @@ object Doc {
* Collapse a collection of documents into one document, delimited
* by a separator.
*
* This works identical to the following code, but is much
Copy link
Contributor

Choose a reason for hiding this comment

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

Small grammar problem, should be "This works identically to the following code..." or possibly "This is equivalent to the following code"

import Doc._
val b0 = Concat(Text("b"), Union(Text(" "), FlatAlt(Line, Text(" "))))
val c0 = Union(Concat(Text("b"), Text(" ")), Concat(Text("b"), FlatAlt(Line, Text(" "))))
law(b0, c0)
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't commit this "raw AST" example -- I'd prefer to either delete it or use our combinators to build it up.

We don't want this as part of our public API, and we weren't testing
it extensively.
@non
Copy link
Contributor

non commented Sep 12, 2019

I pushed a few more changes, I'm 👍 on everything else. @johnynek want to take a look and merge if OK?

@johnynek
Copy link
Collaborator Author

everything looks good!

@non
Copy link
Contributor

non commented Sep 12, 2019

Great! 💯

@non non merged commit d8d582b into master Sep 12, 2019
@non non deleted the erik/hardline-wip branch September 12, 2019 19:06
@vegansk
Copy link

vegansk commented Sep 13, 2019

What about release? I'm using fork because of lack of this feature, and I want to move back to vanilla :-)

@johnynek
Copy link
Collaborator Author

We will release in a few days after we get ANSI color codes in see #184

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.

Non flattenable line to escape .grouped
6 participants