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

Add always_dangle_first_parg #252

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bukepo
Copy link

@bukepo bukepo commented May 8, 2021

This commit adds a new configuration in format so that the first parg
can be dangled at the statement line. This is a common style which is
also used in CMakeLists.txt of cmake itself.

@bukepo bukepo force-pushed the format/always-dangle-first-parg branch from 295b2ab to 1ca315f Compare May 9, 2021 01:13
@bukepo bukepo closed this May 9, 2021
@bukepo bukepo reopened this May 9, 2021
This commit adds a new configuration in format so that the first parg
can be dangled at the statement line. This is a common style which is
also used in CMakeLists.txt of cmake itself.
@bukepo bukepo force-pushed the format/always-dangle-first-parg branch from 1ca315f to 5e9f523 Compare May 9, 2021 01:33
@bukepo bukepo closed this May 9, 2021
@bukepo bukepo reopened this May 9, 2021
@bukepo
Copy link
Author

bukepo commented May 9, 2021

The CI is broken.

@cheshirekow
Copy link
Owner

Looks like maybe a flake8 dependency isn't pinned on CI and got updated. I'll check into it.

@cheshirekow
Copy link
Owner

If I understand the intent of the feature correctly, is this duplicating the existing always_wrap option?
https://cmake-format.readthedocs.io/en/latest/configopts.html#always-wrap

@cheshirekow
Copy link
Owner

cheshirekow commented May 9, 2021

Oh I see, I think you want to make the first argument not wrap (based on the test). So this has been requested dozens of times. As a general rule the "list of commands which" style options are a bit brittle because it leads to different behavior at the top of the tree (statement) than deeper in the tree (e.g. a kwarg) and so leads to a frustrating user experience (I can configure it here, but not there?).

There are some other tricky design considerations for this feature. Instead of requiring it to be enabled "per-statement" my implementation of the feature enables it for any statement where the first positional argument group is "small" (e.g. one or two args). I also considered enabling it if the first positional group would fit in the space available (any number) without wrapping. Would either of those implementations be preferable than what you have provided here?

@bukepo
Copy link
Author

bukepo commented May 10, 2021

I think the main desire is not to wrap the NAME argument. However, the concept doesn't exist in the current model.

And I only want to apply the style to targets related commands, so that we can get a layout very much like a c++ class, where the class name is separated from public/private/protected members.

I like the idea to make such style options available to all nodes in the tree, but only when it's possible to select part of those nodes to apply. Have you considered introducing something like css selector which allows users to apply this style to wanted statements or kwargs?

@bukepo
Copy link
Author

bukepo commented May 20, 2021

@cheshirekow would you mind adding this feature first? We can enhance it to a feature that can be applied to any tree node later. I hope to deploy cmake-format in my projects with this feature.

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

Successfully merging this pull request may close these issues.

None yet

2 participants