Introduce lenses for ReportOpts, ReportSpec, CliOpts, etc.

[Xitian9]

With so many nested options, it would greatly improve developer ergonomics to have some lenses in there.

I've put together a few commits to this end, but I don't think the API is ideal yet. Since the options all have names ending in an underscore, I've generated the lenses by dropping the underscore from the end. The main problem with this is that ReportOpts has color_, drop_, empty_, and transpose_, for which dropping the underscore would result in name clashes. My proposal would be to give these slightly longer names to avoid the clash, for example color_ -> showcolor_, drop_ -> dropaccount_ , empty_ -> showempty_, transpose_ -> transposetable_, or something like that. We might also want to rename watch_ and change_ in UIOpts.

I would be interested in hearing opinions on this.

[Xitian9]

@simonmichael Do you have any opinions on the renaming of these option records? Do you think it's likely to cause much breakage?

[simonmichael]

Xitan9, sorry. This is one I have been putting off because I have mixed feelings and it will take some work to have a useful opinion. This has been something to figure out for years and it's great that you have made a start. Why mixed feelings ? I think some use of lenses will be great for experienced developers, I worry that they will obfuscate the code further for new contributors and for future porters/maintainers; it seems like a hard balance, such that we'll have to choose one or the other. Then simply the adoption process seems a big task - lots of churn and potential for confusion with some of the code lensified and some not, unless we figure out a clear policy and plan. Also which lens package - lens, microlens, optics..
I will try to review what you've done and give more feedback here soon.

[Xitian9]

Not a problem. I've used microlens here since that was already in use in the code base – somebody had put it in hledger-ui at some point. I've done minimal refactoring to use the new lenses here, but I'm happy to remove those places I have put it in if you think they add more in indirection than they save in clarity. I think the most obvious place where they clear things up is in Hledger.UI.UIState, where the messes of nested pattern matching and record updating can be significantly simplified.

[simonmichael]

Great. Yes indeed, hledger-ui is probably the most urgent need. I guess we already use microlens somewhere. I expect all will be well.

[simonmichael]

I did a first pass on this.

Likely to cause much breakage - do you mean, in other users of hledger-lib/hledger libs, like hledger-iadd, and custom scripts ? Not sure but I'm guessing they will all break.

I see you add Data.Default in a few places, making the code more concise. I used to use this more but it seems to be out of favour in the community, and I think I agree. It breaks code navigation in IDEs, and makes the code a little harder to understand for the reader. I think perhaps we should avoid it.

Lenses. It feels like they both giveth and taketh away, especially for non-experts. Code can be much more concise, and (with luck) also easier to understand superficially, but at the same time it becomes much harder to understand completely. Generating them with TH also breaks IDE navigation, and reputedly makes compilation slower (I don't know if that's still true or true in all cases). I guess we could avoid both of those issues by defining them explicitly, at the cost of huge boilerplate. Perhaps worth a try to see how it looks though.

The typeclasses I guess are an optional extra, making it possible to use the lenses even more concisely. They too seem to make coding easier and full understanding harder, probably worth it though.

Perhaps we can discuss this more and run through the changes in chat next ? I'll look out for you, or feel free to suggest a time or two and we could schedule it.

[Xitian9]

Re our discussions, I have made the following changes.

1. I have reduced usage of def
2. I have renamed the fields of ReportOpts mostly as suggested above (exception was that I changed drop_ to droplevels_ instead of dropaccount_
3. I have renamed watch_ from UIOpts to watchfiles_
4. I have refactored the code so that change_ is not needed at all, and removed it from UIOpts.
5. I have written out the classy lenses for ReportSpec manually in the last commit.

Writing the lenses out manually is not so bad, and is very doable for all classes except possibly ReportOpts. ReportOpts will be a pain, but not a disaster. Thoughts?

[Xitian9]

Some more comments:

• UIOpts is now pretty anæmic; it's just a CliOpts and a watchfiles_ :: Bool, which is used in exactly one place (in Hledger.UI.Main). This could easily be pulled out and we could do away with UIOpts altogether, doing away with one layer of complication in the options tower.
• If we're defining the lenses manually, we can do some more custom things. In particular, instead of having reportopts :: Lens' ReportSpec ReportOpts, we could try something like reportopts :: Lens ReportSpec (Either String ReportSpec) ReportOpts ReportOpts, defined using some generalisation of updateReportSpecWith. This would mean that lenses would automatically update the ReportSpec query, removing a common source of bugs. I would have to do some investigations to see what this would mean for lens composibility, but it's tempting.

[Xitian9]

Alternately, instead of eliminating UIOpts, we could make it bigger, and pull in all the ui options pulled from rawopts in an ad-hoc way. These include theme, register, and status-toggles.

[Xitian9]

Regarding my second point above, it looks like this can be done with the following definition:

class HasReportSpec a where
    reportSpec :: Lens' a ReportSpec

    reportopts :: Lens a (Either String a) ReportOpts ReportOpts
    reportopts f = getCompose . reportSpec (Compose . reportopts f)
    …

instance HasReportSpec ReportSpec where
    reportSpec = id
    reportopts f rspec = reportOptsToSpec (reportday_ rspec) <$> f (reportopts_ rspec)
    …

The price we pay is that we can no longer have a useful typeclass HasReportOpts, and will instead have to manually compose reportopts . average instead of just calling average, but this seems like a reasonable trade-off.

[Xitian9]

Actually, we could have our cake and eat it too through a more careful choice of the lens definitions. We could have normal ReportOpts lenses for anything which cannot possibly result in an error, and these fancier lenses only for querystring_ and the bare reportopts_. Let me put some thought into the best design for that.

[simonmichael]

How about:

• hledger field names always begin or end with an underscore.
• Fields are spelled _fieldname (lens: fieldname) when we can get away with it (not too clashing, not too confusing).
• Otherwise/when necessary, fields are spelled _tiFieldName (lens: tiFieldName). ti is a unique prefix for the type, usually the lowercase initial(s) of the type name.
• Many fields currently have the legacy fieldname_ spelling (lens: fieldname). We may migrate these to _fieldname later.

[Xitian9]

That sounds like a reasonable proposal. This means we will currently leave field names for ReportOpts and CliOpts unchanged, but will have to migrate ReportSpec. To be clear, should we be migrating to _rsOpts or to _reportopts (i.e. is this a situation where we can ‘get away with it’?

[simonmichael]

We are already using tiFieldName there, and the reportopts/opts/today/query names would seem a bit clashy with local variables (in user scripts if not main hledger code), or with potential other types with similar fields. But you are close to this code, if you think it's justified for these particular fields then I support it.

[Xitian9]

We will still need to rename some of the ReportOpts and InputOpts fields to avoid clashes, as in 22071c7. I presume these naming choices are acceptable?

In that case, I will propose the following renaming for ReportSpec

• rsOpts -> _rsReportOpts, lens name rsReportOpts (this should rarely be needed in practice with classy lenses)
• rsToday -> _rsDay, lens name rsDay (it really just means the report day, which is usually, though not necessarily, today)
• rsQuery -> _rsQuery, lens name rsQuery
• rsQueryOpts -> _rsQueryOpts, lens name rsQueryOpts

As a first step, I will implement the above renamings, and hand-write classy lenses for ReportOpts and ReportSpec. This will use the fancy lens type proposed in #1545 (comment) where necessary. I will not use these lenses anywhere yet, just implement them.

Does this sound good?

[Xitian9]

And I think I'll implement this in a new branch/PR for now, due to almost all the code being changed.

[simonmichael]

22071c7 are the kind of renames I don't love, creating a new set of names for things. I'm thinking about alternatives. The ReportSpec renames are fine.

[simonmichael]

They are the kind of clashy names where the policy says we resort to tiFieldName. We could avoid renaming everything, instead just doing minimal renames on the problem fields - rocolor, rodrop, roempty...

[Xitian9]

Okay, so we would have a mix of naming styles for ReportOpts fields. That is slightly aesthetically displeasing, but reasonable.

Would we use camel case for prefixed fields? My understanding of your proposal is that those would be called roColor, roDrop, roEmpty, etc..

[simonmichael]

Okay, so we would have a mix of naming styles for ReportOpts fields. That is slightly aesthetically displeasing, but reasonable.

Yes it's ugly but it's just two extra characters on a couple of fields - I'd rather try it as a possibly temporary next step, than adding a new set of names.

Would we use camel case for prefixed fields? My understanding of your proposal is that those would be called roColor, roDrop, roEmpty, etc..

That would be the proper `tiFieldName` spelling, but I'm tentatively proposing something in between: `tifieldname`. It'll be less jarring than mixing `fieldname` and `tiFieldName` in the type don't you think.

[simonmichael]

Or we go all the way and switch all ReportOpts fields to tiFieldName but it seems a pity ? Your call.

[Xitian9]

That would be the proper tiFieldName spelling, but I'm tentatively proposing something in between: tifieldname. It'll be less jarring than mixing fieldname and tiFieldName in the type don't you think.

Hmm… I don't think it's significantly less jarring than tiFieldName, and it would necessitate a further migration step if we rename the other fields.

Or we go all the way and switch all ReportOpts fields to tiFieldName but it seems a pity ? Your call.

This would be my inclination, but I think experience has shown that I'm more willing to make breaking changes. I think these are pretty safe. If it's my call I'd go with this, but I'm happy for you to rein me in.

If we do go this route it will cause a lot of conflicts with other PRs. I would recommend merging any other PRs which are ready or close to it, then creating a PR implementing just the renamings and merging it quickly.

[simonmichael]

ReportOpts is a special case because: - It's a large type, with many fields - it's used everywhere, so clarity, brevity, and avoiding unnecessary churn are more important than usual - many of the fields correspond to UI flags, and we'd like to preserve that correspondence, so renaming is less desirable than usual Importing qualified is an alternative to renaming, but probably too cumbersome. If we must rename I prefer it to be a consistent predictable change rather than different names. Like prepending type initials, or appending two underscores, or appending a not-too-distracting suffix like flag (emptyflag, dropflag, colorflag..) Let's sleep on it.

[Xitian9]

There are several good options there. If we're not renaming everything, appending flag to the relevant options would be a good choice (except that drop is not a flag, but I'm not fussed about that).

[simonmichael]

As general question, is it good or bad for (a) fields and (b) lenses to be visually distinct from other kinds of bindings (functions, "variables") ?

I would guess it's good, because fields and lenses are used in specific ways, and it's helpful for comprehension and to avoid collisions if they follow some consistent, low-noise naming convention. I don't have enough lens experience to really know, though.

If it's good, here are some possibilities (field and lens are the same name here, they could be lowercase, camelCase or whatever):

• field_, lens__
• _field, lens_
• _field, lensl/lensL

PS isn't --drop a flag ? (Personally, I call them options if they have an argument and flags if not.)

[Xitian9]

As general question, is it good or bad for (a) fields and (b) lenses to be visually distinct from other kinds of bindings (functions, "variables") ?

If it's good, here are some possibilities (field and lens are the same name here, they could be lowercase, camelCase or whatever):

* `field_`, `lens__`

* `_field`, `lens_`

* `_field`, `lensl`/`lensL`

The only widely-used convention that I'm aware of is _field and lens. Here are some justifications for that convention by Edward Kmett. I think the idea is that when they exist, lenses are used much more commonly than bare functions, and so have the more idiomatic name.

PS isn't --drop a flag ? (Personally, I call them options if they have an argument and flags if not.)

--drop takes an argument, an integer specifying how many levels to drop from the beginning of the account names.

[simonmichael]

Great link! Ok then let's follow that advice. So how about:

• preferred naming is either _field/lens, or when necessary/preferred, _tiFieldName/tiLensName. Chosen per type.

• when lens names are too clashy, we add a _ suffix.

• ReportOpts has its own convention for the moment: field_/lens. When lens is too clashy, we add a double __ suffix. Later, we'll migrate it to the preferred scheme.

• minimal renaming of existing things at this stage.

Right you are about --drop. I was using "flag" loosely. opt could be a more generic suffix for xOpts fields. I think we can get by without it though.

[Xitian9]

That sounds good. Just to make sure I understand, for the clashy ReportOpts names, are we appending flag to the field name, or appending two underscores to the lens name?

• empty_ -> empty_/empty__
• empty_ -> emptyflag_ / emptyflag

Agreed, I'm happy to play fast and loose with the term flag.

[simonmichael]

I think empty_/empty__ for now.

Later we'll most likely rename to _empty/empty_ I expect, or use opt or flag.

[Xitian9]

Great. I think we've decided on the scope for the next PR. I'll provide a link to that when I've completed it.

Since the scope still includes renaming the fields of ReportSpec, it may still be wise to hold off on this until we merge some of the other PRs, then do a quick PR which is just the renaming, to avoid conflicts.

[simonmichael]

Great! Thanks Stephen.

[Xitian9]

Just recording this for my own information.

The only PR which seem to have significant conflicts with a renaming of the fields of ReportSpec is #1579.

#1554 also has significant conflicts, but it is exploration and unlikely to be merged soon; I think that one is not a blocker.

#1593 has a very minor conflict, but as that PR may not be in final form it may get worse.

[Xitian9]

I have submitted #1624 to rename the fields of ReportSpec.

I just realised, based on InputOpts and CliOptions not being explicitly exempted from the proposed naming scheme, should I rename those fields as well? e.g. output_format_ -> _output_format or coOutputFormat, ignore_assertions_ -> _ignore_assertions or ioIgnoreAssertions?

[simonmichael]

Generally let's do the minimum renaming at this step. We can keep the legacy spelling for these types as well for now.

[Xitian9]

With the merging of #1647, and the submission of #1676, all of the material from this PR is now merged or in other submitted PRs. There remains documentation which refers to discussion here, but this PR can now be closed.