Conversation
nmoroze
left a comment
nmoroze
left a comment
There was a problem hiding this comment.
Thanks for converting this to a PR. I left a few comments after a closer look!
src/tclint/config.py
Outdated
| config_group.add_argument( | ||
| "--spaces-in-braces", | ||
| type=_argparsify(_validate_style_spaces_in_braces), | ||
| metavar="<yes|no|balanced-yes|balanced-no>", |
There was a problem hiding this comment.
Continuing to nitpick on naming, what do you think of a set of names like: always|never|balanced-yes|balanced-no? I'm thinking "always" and "never" perhaps make the distinction between those two options and the "balanced" options more self-explanatory.
There was a problem hiding this comment.
I like it a bit better than the current scheme.
I'm proposing though to hash out the code parts first, and at the point that there are no more comments on the code, do the rename.
There was a problem hiding this comment.
Code looks good to me! Happy to do the rename now, and then we can merge.
There was a problem hiding this comment.
Thanks, I've done the rename.
Also I noticed that --no-spaces-in-braces still used store_false after the enum transition, so I've used store_const there now instead.
vries
force-pushed
the
balance-spaces-in-braces-2
branch
2 times, most recently
from
8c1afb7 to
ff25f2f
Compare
Introduce a new CLI syntax --spaces-in-braces=always/never, replacing --spaces-in-braces/--no-spaces-in-braces. At the CLI level, --no-spaces-in-braces is kept as alias of --spaces-in-braces=never. At the config file level, spaces-in-braces = true/false is also kept as alias of spaces-in-braces = "always"/"never". Consequently, no test-case update is required.
Change style_spaces_in_braces from bool to enum, in preparation of it having more than two values.
Add --spaces-in-braces=balanced-yes/balanced-no, in addition to --spaces-in-braces=always/never. The options are used to set existing variable spaces_in_braces and new variable balanced_spaces_in_braces like so: ``` ------------------------------------------------------------------- name | spaces_in_braces | balanced_spaces_in_braces ------------------------------------------------------------------- never | 0 | 0 always | 1 | 0 balanced-no | 0 | 1 balanced-yes | 1 | 1 ------------------------------------------------------------------- ``` Variable balanced_spaces_in_braces is unused in this commit, but will be used in the following commmit.
Currently this piece of code:
```
if {0} { }
```
is formatted by tclfmt as either (--spaces-in-braces=always):
```
if { 0 } { }
```
or (--spaces-in-braces=never):
```
if {0} {}
```
Add a new 'balanced' mode in which the code is kept as is.
However, in this mode we still want to correct unbalanced space, for instance:
```
if { 0} {}
```
Since there are two possible ways to correct this, we introduce
--spaces-in-braces=balanced-yes and --spaces-in-braces=balanced-no.
With --spaces-in-braces=balanced-yes we get:
```
if { 0 } {}
```
and with --spaces-in-braces=balanced-no instead:
```
if {0} {}
```
Fixes issue:
- nmoroze#138
vries
force-pushed
the
balance-spaces-in-braces-2
branch
from
ff25f2f to
43f8685
Compare
|
Thanks for getting this one over the finish line through a bunch of iterations! I have a few local changes from a while back I want to dust off, and then I'm thinking of cutting another release (hopefully within the next ~week). |
2 participants
Currently this piece of code:
is formatted by tclfmt as either (--spaces-in-braces=yes):
or (--spaces-in-braces=no):
Add a new 'balanced' mode in which the code is kept as is.
However, in this mode we still want to correct unbalanced space, for instance:
Since there are two possible ways to correct this, we introduce
--spaces-in-braces=balanced-yes and --spaces-in-braces=balanced-no.
With --spaces-in-braces=balanced-yes we get:
and with --spaces-in-braces=balanced-no instead:
Fixes issue:
@vries
vries committed