serokell / universum

:milky_way: Prelude written in @Serokell
MIT License
176 stars 26 forks source link

[#290] Add `Universum.Lens` module #291

Open Sorokin-Anton opened 1 year ago

Sorokin-Anton commented 1 year ago

Description

Problem: As in #290, we want to export lens-related operators like (^.) or (^?), but we don't want to depend on microlens or lens.

Solution: Implement those operators by ourselves, and export them in a separate module to avoid name conflicts. Remove those operators from Universum reexports.

Related issues(s)

Fixed #290

✓ Checklist for your Pull Request

Ideally a PR has all of the checkmarks set.

If something in this list is irrelevant to your PR, you should still set this checkmark indicating that you are sure it is dealt with (be that by irrelevance).

Related changes (conditional)

Stylistic guide (mandatory)

✓ Release Checklist

Sorokin-Anton commented 1 year ago

@gromakovsky

  1. It looks like you partially deleted deprecated stuff. I propose to delete all deprecated microlens stuff in this PR

Do we want to have _1, _2, etc in Universum.Lens? I like it and looks like we can just copy-paste the Field1 and other typeclasses.

What about microlens-mtl? I suggest just stopping exporting them, but maybe we still want them?

  1. Since everything in Universum.Lens is now defined manually, I think tests are needed.

Ok will add this week

gromakovsky commented 1 year ago

Do we want to have _1, _2, etc in Universum.Lens? I like it and looks like we can just copy-paste the Field1 and other typeclasses.

AFAIU they won't be compatible with functions from lens or microlens, because they define their own Field1 and other classes. Also they seem to be less trivial and more cumbersome. I'd like to avoid excessive copy-paste and to keep Universum.Lens small, because the whole universum should be relatively lightweight (since it's just a prelude).

But my main point was that currently the code in this branch hardly makes sense: the main point of adding Universum.Lens is to have some lens-related definitions without any extra dependencies, but microlens remains a dependency. So I think all microlens stuff should be deleted prior to adding Universum.Lens. Also you deleted mentions of microlens from README, but it's still used.

Sorokin-Anton commented 1 year ago

Yes I was going to delete them after we'd agree to do so. Let's keep the current exports from Universum.Lens (no microlens-mtl, no lens for tuples). I'll do this, add tests and fix docs where needed soon

Sorokin-Anton commented 1 year ago

I should not change any version tags, right? We must be sure no 1.8.x release will happen after we merge this to master

gromakovsky commented 1 year ago

Yep, don't change the version, but please write about this change in CHANGES.md. The person making the next release is supposed to read the changelog and realize that the change you made here is a breaking one. You may also provide a Migration guide to make it clearer (the migration is to use either lens or microlens as a dependency).