[vim/vim] patch 9.2.NNNN: duplicated sub-option name check in :set completion (PR #20676)
Shane Harper
Problem: The same sub-option name check appears ten times.
Solution: Extract the check into new completing_value_for_subopt() function (Shane Harper).
No functional change.
Previously is_borderhighlight in expand_set_popupoption() was always false: STRNCMP was given "highlight:" instead of "borderhighlight:" as its second argument. There was no user-visible problem with this: is_highlight was already true for "borderhighlight:" (it ends in "highlight:") and the completions for both "borderhighlight:" and "highlight:" are the same.
closes: #NNNN
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/20676
Commit Summary
- 0bd1b67 patch 9.2.NNNN: duplicated sub-option name check in :set completion
File Changes
(1 file)
- M src/optionstr.c (60)
Patch Links:
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Shane Harper
shaneharper left a comment (vim/vim#20676)I intend to create another patch to address a minor problem that existed prior to this patch.
# The following fails. There should be no completions for "invalid_close:" but completions
# for "close:" (['on', 'off']) are generated.
call assert_equal([], getcompletion('set completepopup=invalid_close:', 'cmdline'))
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
h_east
h-east left a comment (vim/vim#20676)- This changes completion-behaviour code but adds no test. Add a regression test that actually fails before this change, i.e. the bogus sub-option case:
call assert_equal([], getcompletion('set diffopt=Xalgorithm:', 'cmdline'))
call assert_equal([], getcompletion('set completepopup=invalid_close:', 'cmdline'))
- Since the helper concentrates the sub-option check in one place, please also anchor it to a sub-option boundary so invalid_close: / Xalgorithm: stop matching, fixing the issue you noted in the same patch.
- completing_value_for_subopt() takes a runtime char *, so STRLEN() can no longer fold to a compile-time constant; the existing inline form could use STRLEN_LITERAL().
- The name completing_value_for_subopt() overstates what it does: it is a plain suffix match, so it returns true for invalid_close: too. Either anchor it to a boundary (see above) so the name holds, or give it a name that reflects the suffix check.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
h_east
h-east left a comment (vim/vim#20676)Yes, I understand.
With that in mind, I am proposing that we handle the series of corrections in this PR.
I don't think it is necessary to break the changes down into such small increments.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.