[vim/vim] Correct enum indentation issue #16289 (PR #16293)
Jim Zhou
Specially handle commas within an enum block to prevent incorrect indentation.
Added constant string STARTS_ENUM to match where enum starts and function CacheEnumBlock to handle IsInside() call.
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/16293
Commit Summary
- 99bafea Correct enum indentation issue #16289
File Changes
(1 file)
Patch Links:
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY pushed 1 commit.
- 02e5a65 Remove a useless conditional statement
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
Don't forget to update runtime/indent/testdir/vim.{in,ok}
tests to test your changes. You can peek at my changeset to
figure out what additional changes are required for CI (also
see this README.txt).
Then, test it locally as follows:
cd runtime/indent
make clean test
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
I would also consider adding a more complicated test, e.g.:
" START_INDENT enum Digits INVALID(v:numbermax), # The null value. ZERO(0 * v:numbermin), ONE(2 - 1), TWO(1 + 1), THREE(9 / 3), FOUR(1 * 4), FIVE(1 + 2 + 2), SIX(36 / 3 / 2), SEVEN(7), EIGHT(2 * 2 * 2), NINE(3 + 3 + 3) const value: number def new(this.value) enddef endenum " END_INDENT
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY pushed 1 commit.
- 37c56ae Place the special handling of the enum block in a more appropriate position.
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
@zzzyxwvut commented on this pull request.
In runtime/autoload/dist/vimindent.vim:
> @@ -461,7 +465,7 @@ export def Expr(lnum = v:lnum): number # {{{2
return startindent + shiftwidth()
endif
endif
-
+
Trailing whitespace characters.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
> @@ -588,6 +592,12 @@ export def Expr(lnum = v:lnum): number # {{{2
return base_ind
endif
endif
+
Ditto.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
> @@ -588,6 +592,12 @@ export def Expr(lnum = v:lnum): number # {{{2
return base_ind
endif
endif
+
+ if line_A.text =~ STARTS_ENUM
+ line_A->CacheEnumBlock()
+ elseif line_A.lnum->IsInside('EnumBlock')
+ return shiftwidth()
Ditto.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
> @@ -314,6 +314,10 @@ const STARTS_CURLY_BLOCK: string = '\%('
const STARTS_FUNCTION: string = $'^\s*\%({MODIFIERS.def}\)\=def\>!\=\s\@='
+# STARTS_ENUM {{{3
+
+const STARTS_ENUM: string = $'^\s*enum\>\s*'
Do you really need string interpolation here?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
> @@ -839,6 +849,25 @@ def CacheBracketBlock(line_A: dict<any>) # {{{2
RegisterCacheInvalidation()
enddef
+def CacheEnumBlock(line_A: dict<any>) # {{{2
+ if line_A.text !~ '^\s*enum\s'
+ return
+ endif
+
+ var pos: list<number> = getcurpos()
+ var startlnum: number = line_A.lnum + 1
+ var endlnum: number = search($'^\s*endenum$', 'nW') - 1
Do you really need string interpolation here?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY pushed 1 commit.
- 0202aa8 Modified test for enum
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
> @@ -588,6 +592,12 @@ export def Expr(lnum = v:lnum): number # {{{2
return base_ind
endif
endif
+
+ if line_A.text =~ STARTS_ENUM
⬇️ Suggested change
- if line_A.text =~ STARTS_ENUM + if line_A.text =~# STARTS_ENUM
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
> @@ -839,6 +849,25 @@ def CacheBracketBlock(line_A: dict<any>) # {{{2
RegisterCacheInvalidation()
enddef
+def CacheEnumBlock(line_A: dict<any>) # {{{2
+ if line_A.text !~ '^\s*enum\s'
⬇️ Suggested change
- if line_A.text !~ '^\s*enum\s' + if line_A.text !~# '^\s*enum\s'
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY pushed 1 commit.
- 8737bb8 Modified test as @zzzyxwvut suggested
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY pushed 1 commit.
- 0ce43aa Remove trailing whitespace characters.
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY commented on this pull request.
In runtime/autoload/dist/vimindent.vim:
> @@ -461,7 +465,7 @@ export def Expr(lnum = v:lnum): number # {{{2
return startindent + shiftwidth()
endif
endif
-
+
Removed. Thanks!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
@zzzyxwvut commented on this pull request.
In runtime/autoload/dist/vimindent.vim:
> @@ -839,6 +849,25 @@ def CacheBracketBlock(line_A: dict<any>) # {{{2
RegisterCacheInvalidation()
enddef
+def CacheEnumBlock(line_A: dict<any>) # {{{2
+ if line_A.text !~ '^\s*enum\s'
+ return
+ endif
+
+ var pos: list<number> = getcurpos()
+ var startlnum: number = line_A.lnum + 1
+ var endlnum: number = search($'^\s*endenum$', 'nW') - 1
Comments can follow endenums, so '^\C\s*endenum\>'; else,
you run the risk of matching some unrelated endenum.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
> @@ -314,6 +314,10 @@ const STARTS_CURLY_BLOCK: string = '\%('
const STARTS_FUNCTION: string = $'^\s*\%({MODIFIERS.def}\)\=def\>!\=\s\@='
+# STARTS_ENUM {{{3
+
+const STARTS_ENUM: string = $'^\s*enum\>\s*'
This pattern should also account for export.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
The proposed changes work for reformatting the whole enum
block and as long as the indent cache is valid. As before,
typing in enum values and keeping them aligned requires one
i_Ctrl-d stroke after entering the second value followed by
a comma.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Jim Zhou
The proposed changes work for reformatting the whole
enum
block and as long as the indent cache is valid. As before,
typing inenumvalues and keeping them aligned requires one
i_Ctrl-dstroke after entering the second value followed by
a comma.
That's indeed a problem, I'll take some time finding a better solution.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY pushed 1 commit.
- f0549fe - Update test to a more complex one.
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY pushed 1 commit.
- 96230de Clean trial blank line
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY pushed 1 commit.
- c418cd3 Use buffer vriable instead of global variable
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY pushed 1 commit.
- 0d4107e Better solution
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY pushed 1 commit.
- 901c78c Merge branch 'master' into master
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY commented on this pull request.
In runtime/autoload/dist/vimindent.vim:
> @@ -839,6 +849,25 @@ def CacheBracketBlock(line_A: dict<any>) # {{{2
RegisterCacheInvalidation()
enddef
+def CacheEnumBlock(line_A: dict<any>) # {{{2
+ if line_A.text !~ '^\s*enum\s'
+ return
+ endif
+
+ var pos: list<number> = getcurpos()
+ var startlnum: number = line_A.lnum + 1
+ var endlnum: number = search($'^\s*endenum$', 'nW') - 1
I am not very familiar with Vim’s regular expressions. Thank you for the guidance. I've corrected it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY pushed 1 commit.
- c0f25c1 Match and more precisely
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jim Zhou
I believe this is a simple solution to the issue. However, since it is limited to enum blocks and partially overlaps with the logic for indenting bracket blocks, further refactoring may be needed in the future to improve the code.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
@zzzyxwvut commented on this pull request.
In runtime/autoload/dist/vimindent.vim:
> + var enum_pos = search('^\C\s*enum\>\s', 'bnW')
+ var endenum_pos = search('^\C\s*endenum\>\s', 'bnW')
⬇️ Suggested change
- var enum_pos = search('^\C\s*enum\>\s', 'bnW')
- var endenum_pos = search('^\C\s*endenum\>\s', 'bnW')
+ var enum_pos = search('^\C\s*\%(export\s\)\=\s*enum\s\+\S\+', 'bnW')
+ var endenum_pos = search('^\C\s*endenum\>', 'bnW')
The flag combination for search() can be tricky; the above
code seems to follow in PrevCodeLine()'s footsteps. If the
cursor is inside EnumBlock, shouldn't it seek forward for
endenum and backward for enum? Will it work when there is
more than one enum definition in the same file?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
> @@ -1078,6 +1052,24 @@ def ContinuesBelowBracketBlock( # {{{3
enddef
def IsInside(lnum: number, syntax: string): bool # {{{3
+ if syntax == 'EnumBlock'
+ var cur_pos = getpos('.')
+ cursor(lnum, 1)
+
+ var enum_pos = search('^\C\s*enum\>\s', 'bnW')
+ var endenum_pos = search('^\C\s*endenum\>\s', 'bnW')
+
Trailing whitespace characters.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
@zzzyxwvut commented on this pull request.
In runtime/indent/testdir/vim.in:
> def new(this.value) +return this.value⬇️ Suggested change
-def new(this.value) -return this.value +def new(value: number) +this.value = value
This is a constructor definition (:help E1365).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
@zzzyxwvut commented on this pull request.
In runtime/indent/testdir/vim.ok:
> def new(this.value) + return this.value⬇️ Suggested change
- def new(this.value) - return this.value + def new(value: number) + this.value = value
This is a constructor definition (:help E1365).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
The latest patch version correctly aligns enum values when
they are typed and when they are formatted. Thank you.
It would be nice if some future patch tried a bit harder at
recovering indentation for value arguments split across two
or more lines when they are typed. For example:
vim9script enum RGB RED( 0xff0000, 0), GREEN( 0x008000, 0), BLUE( 0x0000ff, 0) const value: number var other: any def new(value: number, other: any) this.value = value this.other = other enddef endenum
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
h_east
@h-east commented on this pull request.
In runtime/indent/testdir/vim.in:
> def new(this.value) +return this.value
@zzzyxwvut def new(this.value) is a valid constructor syntax. However, neither return nor assignment is required.
See :h E1390.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY pushed 1 commit.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY commented on this pull request.
In runtime/indent/testdir/vim.ok:
> def new(this.value) + return this.value
Modified. Thank you for pointing that out.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY pushed 1 commit.
- 70f8645 Update runtime/indent/testdir/vim.ok
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY pushed 1 commit.
- eb0b5cc Remove trailing whitespace characters
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY commented on this pull request.
In runtime/autoload/dist/vimindent.vim:
> @@ -1078,6 +1052,24 @@ def ContinuesBelowBracketBlock( # {{{3
enddef
def IsInside(lnum: number, syntax: string): bool # {{{3
+ if syntax == 'EnumBlock'
+ var cur_pos = getpos('.')
+ cursor(lnum, 1)
+
+ var enum_pos = search('^\C\s*enum\>\s', 'bnW')
+ var endenum_pos = search('^\C\s*endenum\>\s', 'bnW')
+
Removed for consistency.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jim Zhou
> + var enum_pos = search('^\C\s*enum\>\s', 'bnW')
+ var endenum_pos = search('^\C\s*endenum\>\s', 'bnW')
For the export enum case I think we should also map 'enum' with 'export' modifier. It is previously uncovered.
--- a/runtime/autoload/dist/vimindent.vim +++ b/runtime/autoload/dist/vimindent.vim @@ -172,6 +172,7 @@ const MODIFIERS: dict<string> = { def: ['export', 'static'], class: ['export', 'abstract', 'export abstract'], interface: ['export'], + enum: ['export'], }
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY commented on this pull request.
In runtime/autoload/dist/vimindent.vim:
> + var enum_pos = search('^\C\s*enum\>\s', 'bnW')
+ var endenum_pos = search('^\C\s*endenum\>\s', 'bnW')
If the
cursor is inside EnumBlock, shouldn't it seek forward for
endenum and backward for enum?
No. This logic may fail. Consider a case
enum Color Red, Green, Blue endenom # <-- Missed var foo: number = 1 # <-- This line may fail enum Subject # <-- Missed Math, Physics endenum
In this case, If we seek for enum backward and endenum forward from the marked line, we might miss the endenum right above and the enum right below. As a result, the function could incorrectly conclude that the marked line is inside an EnumBlock.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY pushed 1 commit.
- 8b01a81 Update combination for search()
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY pushed 1 commit.
- 11b5e3b add modifier DYLD_LIBRARY_PATH=/opt/homebrew/opt/pyt...@3.13/Frameworks/Python.framework/Versions/3.13/lib:
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY pushed 1 commit.
- e78dd1e add '\s' back to search endenum
You are receiving this because you are subscribed to this thread.
Jim Zhou
Updated vim.in and vim.out as @zzzyxwvut suggested.
Updated flag combination to search export enum (as @zzzyxwvut suggested) and MODIFIER to support export enum usage.
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
h_east
@h-east commented on this pull request.
In runtime/indent/testdir/vim.ok:
> def new(this.value) + return this.value
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY pushed 1 commit.
- 0a1f940 Corect search and compare logic
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Jim Zhou
@JimZhouZZY commented on this pull request.
In runtime/autoload/dist/vimindent.vim:
> + var enum_pos = search('^\C\s*enum\>\s', 'bnW')
+ var endenum_pos = search('^\C\s*endenum\>\s', 'bnW')
I just noticed that I did make a mistake in the comparison expression.
(I thought the return value when searching backward was relative to the current position… Bruh…)
Thank you again for making me rethink.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
thanks!
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Christian Brabandt
—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
@zzzyxwvut commented on this pull request.
In runtime/indent/testdir/vim.in:
> def new(this.value) +return this.value
The suggested test had made use of that syntax. However,
with a newer commit, additional indentation was introduced
for testing with a return this.value statement; and return
with a value is malformed syntax for constructors. In order
to keep that indentation and use valid syntax, the above
suggestion was offered.
Otherwise, constructors equally support shorthand assignment
and/or regular parameters, as well as returns with no value.
For example,
vim9script enum Test INSTANCE(1260, "twelve hundred and threescore", true) const number: number const string: string def new(this.number, string: string, _) if strlen(string) < 24 this.string = string else return endif enddef endenum
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Aliaksei Budavei
@zzzyxwvut commented on this pull request.
In runtime/autoload/dist/vimindent.vim:
> + var enum_pos = search('^\C\s*enum\>\s', 'bnW')
+ var endenum_pos = search('^\C\s*endenum\>\s', 'bnW')
Fair enough. It escaped my notice that the cursor is only
moved once, hence the need to search in one direction.
There is still some room for improvement.
We can do better by avoiding the second lookup as soon as it
is known that there is no enum above. It is also worth the
effort to verify that the found enum and/or endenum line is
not the current line (remembering about NonCommentedMatch's
timeouts); otherwise, to reject this line for the related
indentation. See #16310.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.