BlankSpruce
Align comments
Hi again,
Consider the following CMakeLists.txt:
add_library(my_lib STATIC
a.cc # This a
bbbb.cc # this is the b library
cc.cc # c library
ddddd.cc # d lib
)
Running gersemi will give me:
add_library(
my_lib
STATIC
a.cc # This a
bbbb.cc # this is the b library
cc.cc # c library
ddddd.cc # d lib
)
is there any way to keep or enforce the alignment of the comments ?
It's not possible and I don't think it's a great idea in general. I'm aware that general rules might not apply to specific instances though. Here's my reasoning why I think it's a brittle idea that I'd rather not have implemented:
-
Line comment above the argument is usually a good alternative because it can "travel" along with arguments whenever you change something in the code. In fact I've made sure that gersemi will keep relative position of comments above argument even when sorting is hinted (see example: before sorting vs after sorting).
add_library( my_lib STATIC # This a a.cc # this is the b library bbbb.cc # c library cc.cc # d lib ddddd.cc ) -
I don't really like the idea that I change one line and due to formatting constraints (except for unavoidable line break) it makes diffs bigger than necessary. I sometimes call it "losing git blame effect" where you need to jump through more hoops to find the origin of the code due to such commits. Imagine that in this code:
add_library( my_lib STATIC a.cc # This a bbbb.cc # this is the b library cc.cc # c library ddddd.cc # d lib )you want to add another file like
FizzBuzzOutputGenerationContextVisitorFactory.ccand for the sake of simplicity let's imagine it's also commented somehow. This would happen if we have--line-length 120:add_library( my_lib STATIC a.cc # This a bbbb.cc # this is the b library cc.cc # c library ddddd.cc # d lib FizzBuzzOutputGenerationContextVisitorFactory.cc # lol )and this would be your commit diff even though conceptually it's just "add one line with new file to compile":
diff --git a/tmp/foo.cmake b/tmp/foo.cmake index a4c9ce4..42129e9 100644 --- a/tmp/foo.cmake +++ b/tmp/foo.cmake @@ -1,8 +1,9 @@ add_library( my_lib STATIC - a.cc # This a - bbbb.cc # this is the b library - cc.cc # c library - ddddd.cc # d lib + a.cc # This a + bbbb.cc # this is the b library + cc.cc # c library + ddddd.cc # d lib + FizzBuzzOutputGenerationContextVisitorFactory.cc # lol )By the way this would be non-issue if this was our day to day experience.
-
Another problem with that idea: should aligning comments be global for the whole command invocation or should it be related somehow to local grouping? Here's the base case, a bit extreme in amount of comments but I need it to illustrate the case:
install( TARGETS FOO # 1 EXPORT BAR # 2 OBJECTS # 3 DESTINATION BAZ # 4 PERMISSIONS OWNER_READ # 5 CONFIGURATIONS Debug # 6 COMPONENT FOO # 7 NAMELINK_COMPONENT BAR # 8 OPTIONAL # 9 EXCLUDE_FROM_ALL # 10 NAMELINK_ONLY # 11 INCLUDES DESTINATION FOO # 12 )- Global approach:
install( TARGETS FOO # 1 EXPORT BAR # 2 OBJECTS # 3 DESTINATION BAZ # 4 PERMISSIONS OWNER_READ # 5 CONFIGURATIONS Debug # 6 COMPONENT FOO # 7 NAMELINK_COMPONENT BAR # 8 OPTIONAL # 9 EXCLUDE_FROM_ALL # 10 NAMELINK_ONLY # 11 INCLUDES DESTINATION FOO # 12 ) - Semi-global based on "semantic level" like keyword, listing, section etc.:
install( TARGETS FOO # 1 EXPORT BAR # 2 OBJECTS # 3 DESTINATION BAZ # 4 PERMISSIONS OWNER_READ # 5 CONFIGURATIONS Debug # 6 COMPONENT FOO # 7 NAMELINK_COMPONENT BAR # 8 OPTIONAL # 9 EXCLUDE_FROM_ALL # 10 NAMELINK_ONLY # 11 INCLUDES DESTINATION FOO # 12 ) - Specific to section but independent of other sections:
install( TARGETS FOO # 1 EXPORT BAR # 2 OBJECTS # 3 DESTINATION BAZ # 4 PERMISSIONS OWNER_READ # 5 CONFIGURATIONS Debug # 6 COMPONENT FOO # 7 NAMELINK_COMPONENT BAR # 8 OPTIONAL # 9 EXCLUDE_FROM_ALL # 10 NAMELINK_ONLY # 11 INCLUDES DESTINATION FOO # 12 )
- Global approach:
Thanks for the detailed answer. Although the case of the git diff is quite compelling, I still believe it's up to the user to choose its own battles. You can still guide users on which path is the recommended one, by provide good default configuration, but provide options so you're not imposing anything.
To be honest, I like the Global approach, it's visually pleasing and easily readable.
Do you think possible to automatically convert the comments into the "comment above line" mode ?
I still believe it's up to the user to choose its own battles. You can still guide users on which path is the recommended one, by provide good default configuration, but provide options so you're not imposing anything.
I agree with that stance to some extent (that's why there is basic extension system) but I also usually implemented things, even if I weren't the primary user, when I saw the benefit. Honestly speaking I don't see it here.
To be honest, I like the Global approach, it's visually pleasing and easily readable.
As briefly highlighted with diff example I don't find it reasonable. If I really really really had to advocate for something it'd rather be aligning based on scope of "thing". There's a risk though that there might appear accidental same alignment of comments from different scope by pure chance that separate scopes have the same amount of letters like:
install(
TARGETS FOO # 1
EXPORT BAR # 2
OBJECTS # 3
DESTINATION BAZ # 4
PERMISSIONS OWNER_READ # 5
CONFIGURATIONS Debug # 6
COMPONENT FOO # 7
NAMELINK_COMPONENT BAR # 8
INCLUDES DESTINATION FOOBA # 12 - accidental alignment with OBJECTS scope
)
Do you think possible to automatically convert the comments into the "comment above line" mode ?
I think that theoretically would be doable but I really need to think it thoroughly because it's somewhere in the same vein as sort+unique hints where I'd rather have users be extra sure on what they're doing. And I anticipate there's an interesting case that's not clear to me would be obvious and elegant (for the sake of discussion let's ignore --line-length):
# before
install(
TARGETS FOO
EXPORT BAR
OBJECTS
DESTINATION BAZ
PERMISSIONS OWNER_READ
# There is already this very long explanation
# about CONFIGURATIONS here that might
# span multiple lines.
CONFIGURATIONS Debug # This comment would travel above Debug but what was author's intention, was it to comment CONFIGURATIONS or just Debug?
# Another long explanation for another
# part of invocation describing
# why OPTIONAL matters.
OPTIONAL # should this comment really travel below that well-written explanation?
INCLUDES DESTINATION FOO
)
# after
install(
TARGETS FOO
EXPORT BAR
OBJECTS
DESTINATION BAZ
PERMISSIONS OWNER_READ
# There is already this very long explanation
# about CONFIGURATIONS here that might
# span multiple lines
CONFIGURATIONS
# This comment would travel above Debug but what was author's intention, was it to comment CONFIGURATIONS or just Debug?
Debug
# Another long explanation for another
# part of invocation describing
# why OPTIONAL matters.
# should this comment really travel below that well-written explanation?
OPTIONAL
INCLUDES DESTINATION FOO
)
I've started wondering about something else: what if there was a support to keep bracket comments on the left side? Currently it doesn't work like that (because comment and argument are put in separate lines) but I could imagine it working in a way that manual alignment is maintained:
add_library(
my_lib
STATIC
#[[This a ]] a.cc
#[[this is the b library]] bbbb.cc
#[[c library ]] cc.cc
#[[d lib ]] ddddd.cc
)
By the way the diff appears like that in vscode:
The alignment is from the "Event Better TOML" extension.
This is probably driven by libbost-python-devel entry which pushes the comments so far.
What I meant with this picture is that the diff is perfectly understandable, versus your previous screenshot that is indeed quite hard to parse visually I admit.
Anyways, feel free to close the issue. I understand your point on why this could not be something that you want in gersemi.
I need to emphasize something here: I'm aware that you can setup diff view so that it reads better. That snippet above was rendered from markdown code block. My point was that with such a thing you see a pull request +5/-4 lines big where in reality it's just +1/-0 (if you're indeed lucky that only one invocation was changed) and git blame won't show relevant information immediately even though it could be avoided.