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 Doc.lineOr #176

Merged
merged 6 commits into from Sep 11, 2019
Merged

Add Doc.lineOr #176

merged 6 commits into from Sep 11, 2019

Conversation

johnynek
Copy link
Collaborator

close #120 cc @DavidGregory084

This addresses some of the issues raised #76 and #117

Namely, I took the same strategy as #117, but do not expose Line at the top level. This allows us to maintain all the invariants we have, but does not give us the feature request of #76

By changing this PR only to remove the sys.error in flatten on Line and allowing Line at the top level, the following laws fail:

- fill matches spec
- a is flat ==> Concat(a, Union(b, c)) === Union(Concat(a, b), Concat(a, c))
- a.flatten == a.flattenOption.getOrElse(a)
- group law
    /**
     * group(x) = (x' | x) where x' is flatten(x)
     *
     * (a | b)*c == (a*c | b*c) so, if flatten(c) == c we have:
     * c * (a | b) == (a*c | b*c)
     *
     * b.grouped + flatten(c) == (b + flatten(c)).grouped
     * flatten(c) + b.grouped == (flatten(c) + b).grouped
     */

Note this is minorly binary incompatible. We will need to bump the minor version here, but it should be source compatible.

Lastly, I want to apologize to anyone who is disappointed at the pace of change here. This code is super highly tested with very strong law coverage, but that also constrains the implementation quite a bit. It seems like we have to give up some of the laws to get a hardLine (feature requested from #76). @seanmcl did some excellent work trying to get this in, and I lost track of it and he possibly lost interest. In the mean time, @olafurpg also started using a fork due to the lack of this feature. It's a bummer. If I had been more engaged we would be further along in this repo.

What I propose is merge this, which is part of @seanmcl 's work, and then we can see explore the minimal set of laws we can lose to expose Line at the top level.

I think flattening and fill seem to be the ones that are the most at risk.

@johnynek johnynek assigned non and unassigned non Sep 11, 2019
@codecov-io
Copy link

codecov-io commented Sep 11, 2019

Codecov Report

Merging #176 into master will decrease coverage by 0.33%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #176      +/-   ##
==========================================
- Coverage   98.18%   97.84%   -0.34%     
==========================================
  Files           4        4              
  Lines         275      279       +4     
  Branches       19       27       +8     
==========================================
+ Hits          270      273       +3     
- Misses          5        6       +1
Impacted Files Coverage Δ
...re/src/main/scala/org/typelevel/paiges/Chunk.scala 98.24% <100%> (+0.03%) ⬆️
core/src/main/scala/org/typelevel/paiges/Doc.scala 98% <100%> (-0.48%) ⬇️

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 8f31e2c...d4c28ee. Read the comment docs.

@non
Copy link
Contributor

non commented Sep 11, 2019

I'm also sorry that I didn't prioritize this more highly. I think your plan makes sense -- hopefully we can figure something out.

👍 on this PR

oscar-stripe and others added 4 commits September 11, 2019 09:13
We need to ensure that the Doc used with lineOr is already flattened.
To do this, we flatten the doc given (which will be very cheap if it
is already flattened).

We also fixed one of the overly-strict laws.
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.

Doc.lineOrSep?
5 participants