Skip to content

runtime(vimtutor): Further improve the vimtutor shell script#15992

Closed
zzzyxwvut wants to merge 1 commit intovim:masterfrom
zzzyxwvut:runtime/vimtutor/20241104
Closed

runtime(vimtutor): Further improve the vimtutor shell script#15992
zzzyxwvut wants to merge 1 commit intovim:masterfrom
zzzyxwvut:runtime/vimtutor/20241104

Conversation

@zzzyxwvut
Copy link
Contributor

@zzzyxwvut zzzyxwvut commented Nov 4, 2024 •
edited
Loading

  • Rewrite the script usage note.
  • Reconcile the usage help output with the manual page entry
    that describes the implementation in that:
    • a language code argument can be used alone or with its
      option key, e.g. vimtutor -l nl or vimtutor nl;
    • a chapter number argument cannot be used without its
      option key, e.g. vimtutor -c 2.
  • Accept only chapters 1 or 2 as valid chapter arguments.
  • Double-quote instances of shell parameter expansion where
    neither pathname expansion nor field splitting is desired.
  • Prefer $(foo) to `foo` for command substitution.
  • Follow a single indentation style (see the modeline).

@zzzyxwvut zzzyxwvut mentioned this pull request Nov 4, 2024
Closed
@zzzyxwvut
Copy link
Contributor Author

zzzyxwvut commented Nov 4, 2024

TODO:

  • The optional leading 0, which can be used for chapter
    numbers less than 10, appears to be non-functional:

    The file ../runtime//tutor/tutor02.utf-8 does not exist.

    (Despite the mentioned problem, I have now changed back
    the regexps so that the help examples can be tried out.)

  • The supported chapter range is too optimistic [01-99].

  • The script currently boasts 3 styles of indentation: tabs,
    2-, 4-space runs. (I have followed whatever style was in
    hand.)

@zzzyxwvut
Copy link
Contributor Author

zzzyxwvut commented Nov 4, 2024

@chrisbra, a non-ASan Linux task is now struggling with
indent tests. Should the check be about HUGE Linux tasks in
general?

vim/runtime/indent/testdir/runtest.vim

Lines 17 to 28 in 5ccac75

" Remember the directory where we started.
let indentDir = getcwd()
cd ../../src/testdir
" Needed for ValgrindOrAsan().
source shared.vim
exe 'cd ' .. fnameescape(indentDir)
if ValgrindOrAsan()
let g:vim_indent = {"searchpair_timeout": 1024}
let g:python_indent = {"searchpair_timeout": 1024}
endif

@chrisbra
Copy link
Member

chrisbra commented Nov 5, 2024

The optional leading 0, which can be used for chapter
numbers less than 10, appears to be non-functional:

Do we really need this? We haven't had a different chapter for more than 20 years and just now added chapter 2. I suppose we can get away with single digit chapters for now.

The script currently boasts 3 styles of indentation: tabs,
2-, 4-space runs. (I have followed whatever style was in
hand.)

I don't have a strong feeling about it, as long as it is consistent. So just pick one please :)

@chrisbra, a non-ASan Linux task is now struggling with
indent tests. Should the check be about HUGE Linux tasks in
general?

Perhaps we should just increase the timeout for the indent tests in general? Or even slightly increase it from 100 to 200 in

vim/runtime/autoload/dist/vimindent.vim

Lines 17 to 18 in 5ccac75

const TIMEOUT: number = get(g:, 'vim_indent', {})
->get('searchpair_timeout', 100)
?

@zzzyxwvut
runtime(vimtutor): Further improve the vimtutor shell script
77e253e 
- Rewrite the script usage note.
- Reconcile the usage help output with the manual page entry
  that describes the implementation in that:
  * a language code argument can be used alone or with its
    option key, e.g. "vimtutor -l nl" or "vimtutor nl";
  * a chapter number argument cannot be used without its
    option key, e.g. "vimtutor -c 2".
- Accept only chapters 1 or 2 as valid chapter arguments.
- Double-quote instances of shell parameter expansion where
  neither pathname expansion nor field splitting is desired.
- Prefer "$(foo)" to "`foo`" for command substitution.
- Follow a single indentation style (see the modeline).
@zzzyxwvut zzzyxwvut force-pushed the runtime/vimtutor/20241104 branch from 499a49e to 77e253e Compare November 5, 2024 19:38
@zzzyxwvut
Copy link
Contributor Author

zzzyxwvut commented Nov 5, 2024

I haven't rebased my topic branch but I see that the manual
page in the master branch no longer talks about 0 padding
for chapter numbers, so there is no need for me to touch it.

@zzzyxwvut zzzyxwvut marked this pull request as ready for review November 5, 2024 20:32
@chrisbra
Copy link
Member

chrisbra commented Nov 6, 2024

thanks!

@chrisbra chrisbra closed this in 715a58f Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@zzzyxwvut @chrisbra