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

[RFC/RDY] vim-tutor-mode changes #7028

Merged
merged 7 commits into from Jul 16, 2017

Conversation

Projects
None yet
2 participants
@fmoralesc
Contributor

fmoralesc commented Jul 15, 2017

Replaces #7017.

@fmoralesc fmoralesc changed the title from vim-tutor-mode changes to [RFC/RDY] vim-tutor-mode changes Jul 15, 2017

@justinmk

This comment has been minimized.

Member

justinmk commented Jul 15, 2017

" Mappings: {{{1

call tutor#SetNormalMappings()
call tutor#SetSampleTextMappings()
" call tutor#SetSampleTextMappings()

This comment has been minimized.

@justinmk

justinmk Jul 15, 2017

Member

should this be deleted?

This comment has been minimized.

@fmoralesc

fmoralesc Jul 15, 2017

Contributor

Nice catch.

@fmoralesc

This comment has been minimized.

Contributor

fmoralesc commented Jul 15, 2017

Yes, this should solve all those I mentioned in #7017.

@fmoralesc fmoralesc force-pushed the fmoralesc:vimtutor-disentangle2 branch from cfc130c to f4d0521 Jul 15, 2017


If you prefer a book, *Practival Vim* by Drew Neil is recommended often.
If you prefer a book, *Practical Vim* by Drew Neil is recommended often (the sequel, *Modern
Vim*, will include material specific to nvim!).

This comment has been minimized.

@justinmk

justinmk Jul 15, 2017

Member

"will include" => "includes", to avoid time-sensitive verbiage.

fmoralesc added some commits Jul 13, 2017

tutor: allow metadata to exist outside of the documents.
this makes 'expect' regions simpler to handle.
tutor: update syntax
sampletext regions no longer supported

make sure tutorExpect is available

don't conceal code region delimiters

@fmoralesc fmoralesc force-pushed the fmoralesc:vimtutor-disentangle2 branch from f4d0521 to 3014a10 Jul 15, 2017

function! tutor#LoadMetadata()
try
let b:tutor_metadata = json_decode(join(readfile(expand('%').'.json'), "\n"))
catch

This comment has been minimized.

@justinmk

justinmk Jul 16, 2017

Member

why not show an error? what happens if there's a problem?

the call to LoadMetadata is guarded by filereadable(), also.

This comment has been minimized.

@fmoralesc

fmoralesc Jul 16, 2017

Contributor

Failing to load the metadata is not a failure condition, because expect regions (which atm are the only elements defined in the .json file, although I considered allowing defining buffer-specific mappings) are optional to the format. At worst, an error loading this will mean that the check marks and highlighting of those regions will not be performed, which is a mere visual glitch. However, as you mention, this will be mainly covered by the filereadable() guard, so the only kind of issue one could expect is some error in decoding the json file itself. I think I put the try guard in place because the errors that json_decode throws are often unhelpful, but I suppose it's better to show them (or a message) anyway in case something goes wrong.

I could remove this guard or do something like this:

let rjson = join(readfile(expand('%').'.json'), "\n")
try
    let b:tutor_metadata = json_decode(rjson)
catch
    echom "tutor: error loading medatada file for tutorial: ". fnamemodify(expand('%'), ':t:r')
endtry

This comment has been minimized.

@justinmk

justinmk Jul 16, 2017

Member

@fmoralesc with the filereadable guard this should never happen, so if it does happen it is better to get as much information as possible. So I would suggest to remove try-catch. Up to you, I don't want to block this PR.

This comment has been minimized.

@fmoralesc

fmoralesc Jul 16, 2017

Contributor

That's fine with me, give me a sec.

@fmoralesc fmoralesc force-pushed the fmoralesc:vimtutor-disentangle2 branch from 3014a10 to 3241bce Jul 16, 2017

@justinmk justinmk merged commit a76da96 into neovim:master Jul 16, 2017

0 of 3 checks passed

QuickBuild Build pr-7028 is started
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment