Closed jidhub closed 1 year ago
Could you ensure all tests passed on your local at first? Here is a guide to run tests:
https://github.com/rhysd/clever-f.vim/blob/master/test/README.md
Sure will do.
Could you ensure all tests passed on your local at first? Here is a guide to run tests:
The test that fails is the test checking that clever_f#_mark_direct() only highlights ASCII chars : "not ok 65 - clever_f#_mark_direct() should not highlight multibyte characters"
Can I update this test, instead of creating a new test ?
There is also a spurious vint[1] error:
autoload/clever_f.vim:160:29: Undefined variable: c: (see :help E738)
c
is defined at line 146: let [_, l, c, _] = getpos('.')
c
is used just above my patch at line 148: if (a:forward && c >= len(line)) || (!a:forward && c == 1)
without problems detected from vint.
How comes that vint suggests c
is undefined at line 160 ?
Can I update this test, instead of creating a new test ?
Of course. Could you fix the test case if its assumption is no longer correct?
How comes that vint suggests c is undefined at line 160 ?
This seems a bug of vint or its dependency vimlparser.
Hm, can you try to modify L160 as follows and check if the error no longer happens or not?:
let line = split(line[c : ], '\zs')
The parser may be confused by c:
since Vim script's variable is prefixed by single alphabet and colon (g:
, l:
, ...).
Oh sorry I use github editor and was prepared to run tests locally afterwards.
Githud says "Changes requested" but I am not finished testing !
I added a backwards test. Now I need to install the testing software on my computer to get some more input.
Merging #74 (b607891) into master (e852984) will increase coverage by
0.14%
. The diff coverage is100.00%
.
@@ Coverage Diff @@
## master #74 +/- ##
==========================================
+ Coverage 93.50% 93.64% +0.14%
==========================================
Files 2 2
Lines 354 362 +8
==========================================
+ Hits 331 339 +8
Misses 23 23
Impacted Files | Coverage Δ | |
---|---|---|
autoload/clever_f.vim | 93.51% <100.00%> (+0.15%) |
:arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
Test passed, this is good to go !
/data/data/com.termux/files/home/.vim/pack/bundle/start/clever-f.vim-mark_direct_beyond_ASCII/test$ ./vim-themis/bin/themis
[....]
# passes 81
Thanks for your review time and insights.
Github still repeats your request to create a new multibyte tescase: https://github.com/rhysd/clever-f.vim/pull/74#pullrequestreview-1141711381
How can I tell github that I did exactly that in https://github.com/rhysd/clever-f.vim/pull/74/commits/ddf8b51403e34b83d2cd04585f33b1def140297b ?
Github still repeats your request to create a new multibyte tescase: https://github.com/rhysd/clever-f.vim/pull/74#pullrequestreview-1141711381
That's because I haven't reviewed this PR again. What you need to do is just requesting me review again. Don't mind, I'll review this PR soon.
Looks good. Thank you for adding tests as well.
Thanks for the additionnal reformatting.
Can you tel me if the following coverage notice is positive or negative ?
## master #74 +/- ##
==========================================
+ Coverage 93.50% 93.64% +0.14%
==========================================
Files 2 2
Lines 354 362 +8
==========================================
+ Hits 331 339 +8
Misses 23 23
Can you share a link about what coverage does ?
It means positive (+8 lines)
ch
, instead of just containing a byte (that is ignored when non-ASCII, e.g., coding utf8 or other encoding), now contains a possibly multibyte char.line
is changed to contain the list of these groupped non-ASCII chars. Tested on utf8.