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

Make Bifoldable docs #4109

Merged
merged 12 commits into from
Jun 4, 2022
Merged

Make Bifoldable docs #4109

merged 12 commits into from
Jun 4, 2022

Conversation

gatear
Copy link
Contributor

@gatear gatear commented Jan 12, 2022

The first draft for Bifunctor docs as required in #1801

@gatear
Copy link
Contributor Author

gatear commented Jan 12, 2022

@armanbilge here it is as promised in #4076 👍

Copy link
Member

@armanbilge armanbilge 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 following up on this!

docs/src/main/mdoc/typeclasses/bifoldable.md Outdated Show resolved Hide resolved
docs/src/main/mdoc/typeclasses/bifoldable.md Outdated Show resolved Hide resolved
docs/src/main/mdoc/typeclasses/bifoldable.md Outdated Show resolved Hide resolved
docs/src/main/mdoc/typeclasses/bifoldable.md Outdated Show resolved Hide resolved
docs/src/main/mdoc/typeclasses/bifoldable.md Outdated Show resolved Hide resolved
@gatear
Copy link
Contributor Author

gatear commented Feb 4, 2022

@armanbilge I would say this version is final and ready for a re-review. Thanks!

armanbilge
armanbilge previously approved these changes Feb 8, 2022
@gatear gatear mentioned this pull request Feb 9, 2022
70 tasks
armanbilge
armanbilge previously approved these changes Feb 21, 2022
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

👍 from me, thanks again! :)

@gatear
Copy link
Contributor Author

gatear commented Feb 22, 2022

@johnynek thanks for your feedback from #4076 🍻 . I followed-up with Bifoldable docs. What do you think about it ?

```scala mdoc
case class Report(entries: List[String], errors: Int) {
def withEntries(entry: String): Report =
this.copy(entries = entries :+ entry)
Copy link
Contributor

Choose a reason for hiding this comment

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

appending to the right of a list is an anti-pattern (because it is O(N) and repeatedly doing it can create quadratic performance issues). Can we use Vector[String] or Chain[String] if you want to append?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

appending to the right of a list is an anti-pattern (because it is O(N) and repeatedly doing it can create quadratic performance issues). Can we use Vector[String] or Chain[String] if you want to append?

@johnynek I've switched to Chain[String]]

def bifoldLeft[A, B, C](fab: F[A, B], c: C)(f: (C, A) => C, g: (C, B) => C): C

//lazily performs a right-associative bi-fold over `fab`
def bifoldRight[A, B, C](fab: F[A, B], c: Eval[C])(f: (A, Eval[C]) => Eval[C], g: (B, Eval[C]) => Eval[C]): Eval[C]
Copy link
Contributor

Choose a reason for hiding this comment

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

can we say any laws these must follow? You have a tuple example below which uses an ordering (left to right in your example), but would the other direction also be lawful?

@gatear gatear requested a review from johnynek March 6, 2022 09:52
@armanbilge armanbilge added this to the 2.8.0 milestone May 16, 2022
@gatear
Copy link
Contributor Author

gatear commented Jun 2, 2022

@johnynek could you take a look? Not sure if I managed to address your comments.

@armanbilge
Copy link
Member

@gatear I'm sorry this has been sitting for so long, but I would like to merge it before our next release. Thanks again for your contribution.

@armanbilge armanbilge merged commit c215f33 into typelevel:main Jun 4, 2022
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.

3 participants