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

Indent rules from clint.py are inconsistent #17763

Closed
kylo252 opened this issue Mar 18, 2022 · 9 comments · Fixed by #18611
Closed

Indent rules from clint.py are inconsistent #17763

kylo252 opened this issue Mar 18, 2022 · 9 comments · Fixed by #18611
Labels
code-standards code style, practices, guidelines, patterns enhancement feature request

Comments

@kylo252
Copy link
Contributor

kylo252 commented Mar 18, 2022

Feature already in Vim?

NA

Feature description

The whitespace/indent rules specifically appear to be either broken or mis-configured

neovim/src/clint.py

Lines 2915 to 2921 in 3830960

# There are certain situations we allow one space, notably for section
# labels
elif ((initial_spaces == 1 or initial_spaces == 3) and
not Match(r'\s*\w+\s*:\s*$', cleansed_line)):
error(filename, linenum, 'whitespace/indent', 3,
'Weird number of spaces at line-start. '
'Are you using a 2-space indent?')

why would the inner expression indentation be set (hard-coded) to 4 when the global indentation is 2?

neovim/src/clint.py

Lines 2118 to 2123 in 3830960

if (pos != depth_line_starts[depth][0] + 4
and not (depth_line_starts[depth][1] == '{'
and pos == depth_line_starts[depth][0] + 2)):
if depth not in ignore_error_levels:
error(filename, linenum, 'whitespace/indent', 2,
'Inner expression indentation should be 4')


Example

src/nvim/shada.c:3319:  Weird number of spaces at line-start.  Are you using a 2-space indent?  [whitespace/indent] [3]
#define CHECK_KEY(key, expected) \
  ((key).via.str.size == sizeof(expected) - 1 \
   && STRNCMP((key).via.str.ptr, expected, sizeof(expected) - 1) == 0)

while adding two extra spaces for each line fixes it

#define CHECK_KEY(key, expected) \
    ((key).via.str.size == sizeof(expected) - 1 \
     && STRNCMP((key).via.str.ptr, expected, sizeof(expected) - 1) == 0)

Neither clang-format nor uncrustify is able to consistently keep up with this, at least I wasn't able to configure them, so we end up alternating between the two tools for each error, with a mixture of manual editing and trial and error to get around this.

@kylo252 kylo252 added the enhancement feature request label Mar 18, 2022
@zeertzjq zeertzjq added the code-standards code style, practices, guidelines, patterns label Mar 18, 2022
@zeertzjq
Copy link
Member

zeertzjq commented Apr 6, 2022

Hmm, this entry in uncrustify.cfg:

indent_continue = 0 # number

does not match this:
- Wrapped parameters have a 4 space indent.

@dundargoc
Copy link
Member

If we set indent_continue = 4 clint will warn about Inner expression should be aligned as opening brace + 1. Not sure how it's possible to satisfy both "this should have an indent of 4" but also "must have 1 space after parenthesis" rule.

@zeertzjq
Copy link
Member

zeertzjq commented Apr 6, 2022

Oh...

@dundargoc
Copy link
Member

Hardcoding the exact alignment of inner expression is probably the single most disruptive style rule of neovim. I've had to sacrifice all formatting of ternary operators in order to satisfy this rule.

@kylo252
Copy link
Contributor Author

kylo252 commented Apr 6, 2022

@dundargoc, semi related, would it be possible to either update or lock uncrustify.cfg to an actual version? I keep getting these complaints

Option<NUM>: at src/uncrustify.cfg:1182: Expected number , for 'indent_shift'; got 'false'
Option<NUM>: at src/uncrustify.cfg:1356: Expected number , for 'indent_comma_brace'; got 'false'
Option<NUM>: at src/uncrustify.cfg:1360: Expected number , for 'indent_comma_paren'; got 'false'
Option<NUM>: at src/uncrustify.cfg:1364: Expected number , for 'indent_bool_paren'; got 'false'

this is what I'm using

$ uncrustify --version
Uncrustify-0.74.0-203-9bdb790e0

and this is the rule

# Uncrustify-0.74.0

I think these rules are "tuneable" as of late.

@dundargoc
Copy link
Member

The uncrustify config is "locked" to 0.74, the latest stable. You can update the config file to a newer uncrustify version by using

uncrustify -c uncrustify.cfg --update-config-with-doc -o uncrustify.cfg

This will update the uncrustify config to a newer version using the options from the old uncrustify config.

@kylo252
Copy link
Contributor Author

kylo252 commented Apr 6, 2022

The uncrustify config is "locked" to 0.74, the latest stable. You can update the config file to a newer uncrustify version by using

uncrustify -c uncrustify.cfg --update-config-with-doc -o uncrustify.cfg

This will update the uncrustify config to a newer version using the options from the old uncrustify config.

thanks for the clarification! I was wondering what the "update" part meant, since I've been basically running this to generate a new file and then merge them manually

uncrustify --update-config-with-doc -o src/uncrustify_new.cfg

even the help output is misleading...

$ uncrustify --help
...
Config/Help Options:
 -h -? --help --usage     : Print this message and exit.
 --version                : Print the version and exit.
 --count-options          : Print the number of available options and exit.
 --show-config            : Print out option documentation and exit.
 --update-config          : Output a new config file. Use with -o FILE.
 --update-config-with-doc : Output a new config file. Use with -o FILE.
 --universalindent        : Output a config file for Universal Indent GUI.
 --detect                 : Detects the config from a source file. Use with '-f FILE'.
                            Detection is fairly limited.
 --set <option>=<value>   : Sets a new value to a config option.
  ...

@justinmk
Copy link
Member

Hardcoding the exact alignment of inner expression is probably the single most disruptive style rule of neovim. I've had to sacrifice all formatting of ternary operators in order to satisfy this rule.

Personally I'd be in favor of relaxing this, if it significantly helps us automate formatting & linting. @bfredl @jamessan ?

@bfredl
Copy link
Member

bfredl commented Apr 25, 2022

Yes. I think it is a good default, but we don't need to strictly enforce it.

dundargoc added a commit to dundargoc/neovim that referenced this issue May 17, 2022
Uncrustify is the source of truth where possible.
Remove any redundant checks from clint.py.
See also neovim#18563

Closes neovim#17763
dundargoc added a commit to dundargoc/neovim that referenced this issue May 17, 2022
Uncrustify is the source of truth where possible.
Remove any redundant checks from clint.py.
See also neovim#18563

Closes neovim#17763
dundargoc added a commit to dundargoc/neovim that referenced this issue May 17, 2022
Uncrustify is the source of truth where possible.
Remove any redundant checks from clint.py.
See also neovim#18563

Closes neovim#17763
dundargoc added a commit to dundargoc/neovim that referenced this issue May 17, 2022
Uncrustify is the source of truth where possible.
Remove any redundant checks from clint.py.
See also neovim#18563

Closes neovim#17763
dundargoc added a commit to dundargoc/neovim that referenced this issue May 17, 2022
Uncrustify is the source of truth where possible.
Remove any redundant checks from clint.py.
See also neovim#18563

Closes neovim#17763
dundargoc added a commit to dundargoc/neovim that referenced this issue May 17, 2022
Uncrustify is the source of truth where possible.
Remove any redundant checks from clint.py.
See also neovim#18563

Closes neovim#17763
justinmk pushed a commit that referenced this issue May 21, 2022
Uncrustify is the source of truth where possible.
Remove any redundant checks from clint.py.
See also #18563

Closes #17763
Shougo pushed a commit to Shougo/neovim that referenced this issue May 22, 2022
…8611

Uncrustify is the source of truth where possible.
Remove any redundant checks from clint.py.
See also neovim#18563

Closes neovim#17763
kraftwerk28 pushed a commit to kraftwerk28/neovim that referenced this issue Jun 1, 2022
…8611

Uncrustify is the source of truth where possible.
Remove any redundant checks from clint.py.
See also neovim#18563

Closes neovim#17763
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-standards code style, practices, guidelines, patterns enhancement feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants