Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong formatting for "install" command #54

Closed
b-nathan opened this issue Jan 22, 2025 · 5 comments
Closed

Wrong formatting for "install" command #54

b-nathan opened this issue Jan 22, 2025 · 5 comments

Comments

@b-nathan
Copy link

gersemi --version
gersemi 0.17.0
lark 1.2.2
colorama (missing)
Python 3.9.2 (default, Dec  1 2024, 12:12:57) 
[GCC 10.2.1 20210110]

This is how gersemi formatts the install section:

install(
    TARGETS
        ${NAME}
        DESTINATION
        bin
)

And this is how I think it should be formatted, according to the CMake Documenation:

install(
    TARGETS
        ${NAME}
    DESTINATION
        bin
)

What rules follows gersemi for formatting and why does it not format as expected?

This is my .gersemirc:

# This is the config file for the CMakeLists.txt file formatter "gersemi"
# yaml-language-server: $schema=https://raw.githubusercontent.com/BlankSpruce/gersemi/master/gersemi/configuration.schema.json

definitions: []
disable_formatting: false
indent: 4
line_length: 80
list_expansion: favour-expansion
unsafe: false
warn_about_unknown_commands: false
@BlankSpruce
Copy link
Owner

AFAICT it was a bug already covered in #51. Try with the newest 0.18.2.

@BlankSpruce
Copy link
Owner

@b-nathan Let me know if the issue is cleared for you with the newest version.

@b-nathan
Copy link
Author

@BlankSpruce first of all thank you very much for developing this tool and especially actively maintain it!

The newest version indeed changes the behaviour and formats the code as follows: (line_length =80). Which is correct based on the rules.

install(TARGETS ${NAME} DESTINATION bin)

When I the set line_length to 20, it looks like this. And I have this scenario at many places with the new update. Commands, which were previously fully extended, are now "half-extended", although favour-expansion is set. I would expect, that with favour-expansion as soon as the line-length is exceeded, it then "expands everything" as it does in the very last example.

install(
    TARGETS
        ${NAME}
    DESTINATION bin
)

When I the set line_length to 5, it finally looks as I want and it makes sense. I then just have to deal with the fact, that all single lines are expanded, which is a bit odd.

install(
    TARGETS
        ${NAME}
    DESTINATION
        bin
)

Can you explain me, why it does not fully expand, when the line-length is once exceeded with 0.18.2?

@BlankSpruce
Copy link
Owner

BlankSpruce commented Jan 27, 2025

Short answer:
DESTINATION is "one value keyword" hence it's expanded into two lines when it's absolutely necessary:

https://github.com/BlankSpruce/gersemi?tab=readme-ov-file#alternative-style-favour-expansion

One-value arguments (like NAME in add_test) will be inlined unless that'd violate character limit.

https://cmake.org/cmake/help/latest/command/install.html#targets

[DESTINATION <dir>]                # one value
[PERMISSIONS <permission>...]      # multi value
[CONFIGURATIONS <config>...]       # multi value
[COMPONENT <component>]            # one value
[NAMELINK_COMPONENT <component>]   # one value
[OPTIONAL] [EXCLUDE_FROM_ALL]      # options
[NAMELINK_ONLY|NAMELINK_SKIP]      # options

However ridiculous -l 20 is it's not enough. 😄 You need at least -l 18 or longer (in number of characters) value of DESTINATION to have that line break:

$ gersemi --list-expansion favour-expansion CMakeLists.txt
install(TARGETS ${NAME} DESTINATION bin)

$ gersemi --list-expansion favour-expansion -l 20 CMakeLists.txt
install(
    TARGETS
        ${NAME}
    DESTINATION bin
)

$ gersemi --list-expansion favour-expansion -l 18 CMakeLists.txt
install(
    TARGETS
        ${NAME}
    DESTINATION
        bin
)

Or if you really really want to have line break you can force it with comment (empty comment is enough), DESTINATION expanded, COMPONENT not expanded:

gersemi --list-expansion favour-expansion CMakeLists.txt 
install(
    TARGETS
        ${NAME}
    DESTINATION #
        bin
    COMPONENT foobar
)

Longer answer:
The idea behind favour-expansion is to in fact favour or in other words prefer expansion in certain situations but it doesn't necessarily mean "expand as much as possible". In particular my goal is to have "healthy" diffs when changes happen to the code so that they are easier (with some but not exact definition of "easy") to read, review or explore, especially explore in terms of utilities like git blame. I think that there isn't much to gain from separating keyword like DESTINATION from it's one value when probably pair KEYWORD value would fit into one line. On the other hand multi value keywords gain from separation in at least these aspects:

  • addition of a single value leads to simple +1/-0 diff
  • each value is git blame-able, you can easily inspect when each of the values was introduced and possibly (when your co-authors make nice commit message) understand the reason for such line
  • on average there's slight visual hint that XYZ is one value keyword but ABC is multi value keyword by mere fact of line breaking, of course this visual hint is lost when XYZ has very long value attached to it that forces line break due line_length setting

I'm not saying this is the perfect* way to format code but I can safely claim there is some consistency and reasoning for current state of favour-expansion.

Longer longer answer:
The whole idea for favour-expansion got created due to this excellent proposal and after a lot of back and forth we got to the current situation and the rules governing that style. Two years later I can easily say that there are some shortcomings of the established rules but overall I'd say the shortcomings are eclipsed by the good enough utility of this style.


* - Nowadays I think style slightly closer to perfection is somewhere in space between favour-inlining and favour-expansion but probably weighted towards favour-expansion. However I don't intent to explore that space because I already have one style too many in the formatter when I look at the original goals:

Formatter should "just work" and should have as little configuration as possible so that you don't have to worry about fine-tuning formatter to your needs

@b-nathan
Copy link
Author

Thank you for the thorough explanation. It really helped to understand, why the code is formatted as it is and I can perfectly live the way it is right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants