ingolemo / python-lenses

A python lens library for manipulating deeply nested immutable structures
GNU General Public License v3.0
308 stars 19 forks source link

API too magic? #2

Closed ingolemo closed 4 years ago

ingolemo commented 8 years ago

The hardest part of writing a library like this is presenting a good pythonic api. While I think that the current api is decent, there are some things that could be better.

I dislike the underscore attributes and how they're not used consistently; it's lens().each_(), but lens().get(). This represents an underlying difference in the implementation, but it might not be a good thing to expose to the user. I may add the underscores to every method of Lens, or remove them. Perhaps I can find a way to separate the building of a lens from the using of one. Part of the problem is that I'm using attribute lookup on the lens to do attribute focusing, which is super convenient, but also sloppy.

I especially dislike the _l attributes. These come from a desire to make functionality easily accessible, but it's not clear to me how useful ZoomAttrLens really is. I may just remove them.

Which is a better api?

from lenses import lens
lens([[1, 2j], 3]).tuple_(lens()[0], lens()[1].wraplist_()).each_().each_().real.get_all()

from lenses import l
l.tuple_(l[0], l[1].wraplist_()).each_().each_().real.get_all(state=[[1, 2j], 3])

The latter just needs l = lens() and is more convenient in many situations but makes bound lenses more difficult to make. How useful are bound lenses? Could do the equivalent of lens = lens() if l is too short.

A few other variations for comparison:

from lenses import lens
lens([[1, 2j], 3]).tuple(lens()[0], lens()[1].wraplist()).each().each().real.get_all()

from lenses import lens, shard
lens([[1, 2j], 3]).tuple_(shard[0], shard[1].wraplist_()).each_().each_().real.get_all_()

import lenses as l
(l.Tuple(l.GetItem(0), l.GetItem(1) & l.WrapList()) & l.Each() & l.Each() & l.GetAttr('real').get_all(state=[[1, 2j], 3])

from lenses import *
(Tuple(lens[0], lens[1] & WrapList()) & Each() & Each() & lens.real).get_all(state=[[1, 2j], 3])

I'm not sure anyone will read this, but it does good to get my thoughts out in the open. Suggestions welcome.

ingolemo commented 8 years ago

After writing out those variations, I'm almost instantly leaning towards getting rid of the underscores. Punctuation is bad for readability.

ingolemo commented 8 years ago

The terrible _l suffix has been excised!

ingolemo commented 7 years ago

Potential issue; Lens.get and Lens.get_ would conflict if I removed the underscores.

ingolemo commented 7 years ago

The current api for this example looks like this:

from lenses import lens
lens.tuple_(lens[0], lens[1].wraplist_()).each_().each_().real.collect()([[1, 2j], 3])

I'm currently considering removing the underscores (renaming get_) and making it so that you have to explicitly access attributes with the getattr_ method (will probably be renamed to attr). This is a little less convenient, but it removes a lot of magic and punctuation.

So you might get something like this:

Box = namedtuple('Box', value)
data = [Box([1, 2, 3]), Box([4, 5]), Box([])]
lens.each().attr('value').each().get()(data)

How common is it that people want to access attributes of their state?

Daenyth commented 7 years ago

What do you think about reserving initial-capital-letter method names to be the lens stuff, and lower case attributes will focus? Eg lens.Tuple(lens[0], lens[1].Wraplist()).Each().Each().real.Collect()([[1, 2j], 3])

ingolemo commented 7 years ago

That's a not a bad idea... I'll experiment with it; see how it feels in my own code.

It's a shame to make such a breaking change immediately after the first proper release, but I suppose that can't be helped. ¯\_(ツ)_/¯

ingolemo commented 7 years ago

I've decided to go ahead with this change. All the lens constructor methods have been capitalised, but regular methods like get, collect, and set have not (at least not yet, maybe in the future). The old underscore names are still available for now, but they'll be removed sooner or later.

ingolemo commented 4 years ago

Not much point leaving this around now that the project is 1.0 and can't change the api.