add optional column alignment in write

[aekiss]

Hi @marshallward, just wondering if you'd consider this PR.

It adds an optional colwidth argument to write and associated functions that sets a minimum column width between the start of a variable name and the start of " = " in the output. This can make output much more readable, e.g. https://github.com/COSIMA/01deg_jra55_iaf/blob/ak-dev/ice/cice_in.nml

It uses ljust, so if a variable name is too long it will simply push the " = " along a bit more (i.e. the variable name won't be trimmed - see history_deflate_level in the link). The usual behaviour is retained with the default colwidth=0.

[coveralls]

Coverage remained the same at 97.683% when pulling 0242929 on aekiss:write-colwidth into 305e61b on marshallward:master.

[marshallward]

It looks like a nice feature. Not my own style, but certainly that of many others I know. And I agree it helps for navigating huse namelists like the one you linked.

Setting a minimum colwidth feels like a confusing and iterative exercise here, with one trying to guess what it should be to fit across all groups. Is that right?. Seems like one wants it to be "enough to align all values across all groups"? Or just a single group?

Also, I have been moving away from adding arguments to the API for features like this, and towards setting properties in the Namelist or Parser, similar to indent or column_width. So perhaps this should be defined as a property rather than a function argument.

I would also include min_ somewhere in the argument name if you do intend for it to be a minimum colum width, and would like to iterate some more on the final name.

[aekiss]

I thought about automatically setting the column width, but I've decided it doesn't suit my purposes, as the gap is too large in MOM input.nml, and auto-width also runs the risk of a single-line change producing whitespace changes throughout the namelist, making it confusing to track namelist changes e.g. with git. Taking the width as a parameter allows the calling code to automatically set it if desired (e.g. how I implemented it here). Auto-fitting on a per-group basis reduces both those problems but would require a more complicated implementation.

Agreed, min_ in the argument name makes sense, e.g. min_colwidth. That also might reduce confusion with column_width.

I'm not sure about making it a property - I tried something like that years ago with the sort argument and we decided against it in the end, as it only affected the output, not the namelist itself - see #50 - Is this a similar situation?

[marshallward]

I don't recall what the problem was with adding sort as a property, but isn't this just a formatting setting? It should be no different from indent or uppercase?

There might be some objection to OO style here, but it feels more consistent to me to add this as a property.

[aekiss]

Yes, it's a formatting setting, the same as sort and those properties you listed.
I could have a look at making min_colwidth a property when I have a spare moment but that won't be for a while.
Note to self: I guess I'd need to make changes analogous to these changes for sort that didn't end up in master aekiss@9148e87

[marshallward]

It's too bad the sorting idea got lost in #50, we should revisit it.

[aekiss]

I don't have an opinion either way re. argument vs. property, just so long as the functionality is available somehow to my application in nmltab.

[marshallward]

Andrew, I apologize for letting this PR slip by. I recently made a change which will split strings which exceed the column limit:

https://github.com/marshallward/f90nml/tree/string_split

which does rework the header, so I expect it will disrupt this PR. I will merge that one today and then come back to this one. My initial thoughts:

• I would like to switch to a property-based configuration to stay in line with the other formatting tools
• col_width is probably far too close to column_wdith, so I am thinking something like align_values or value_align (or somehthing with "align" in it).

Looking over my past comments, I don't think a min_ tag is needed, nor do we need to worry about making it "smart" (not at this stage at least).

[marshallward]

BTW I am currently trying to merge this, so probably no need yet for you to work on it.

[aekiss]

great, thanks @marshallward