8000 fix(pattern): create custom doc only in valid rtp's by Sh3Rm4n · Pull Request #104 · kkoomen/vim-doge · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix(pattern): create custom doc only in valid rtp's #104

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 0 commits into from
Closed

fix(pattern): create custom doc only in valid rtp's #104

wants to merge 0 commits into from

Conversation

Sh3Rm4n
Copy link
@Sh3Rm4n Sh3Rm4n commented Oct 11, 2020 •
edited
Loading

Prelude

By contributing to DoGe you agree to the following statements

Why this PR?

This handles the special case, when .vim runtimepaths are lying around
on the system, but are not added in the runtimepath of the running
(neo)vim instances itself.

I had a ~/.vim folder lying around in my system.
Executing :DogeCreateDocStandard ... opened ~/.vim/after/ftplugin/...,
I use neovim and the ~/.vim folder is no longer included in my runtimepath setup,
so I added this check.

@Sh3Rm4n Sh3Rm4n changed the title fix(pattern): create custom doc only in valid rtp\s fix(pattern): create custom doc only in valid rtp's Oct 11, 2020
kkoomen
kkoomen reviewed Oct 11, 2020
View reviewed changes
autoload/doge/pattern.vim Outdated
@@ -185,6 +185,9 @@ function! doge#pattern#custom(name) abort
let l:path = ''
for l:p in ['~/.vim', '~/vimfiles']
if isdirectory(expand(l:p))
if has('nvim') && match(&runtimepath, expand(l:p)) == -1
Copy link
Owner
@kkoomen kkoomen Oct 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can just do it like this:

if isdirectory(expand(l:p)) && match(&runtimepath, expand(l:p)) >= 0
  " ...
endif

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that looks cleaner.

@Sh3Rm4n Sh3Rm4n force-pushed the master branch 3 times, most recently from a4ea2a6 to 9604d8b Compare October 11, 2020 12:09
@Sh3Rm4n
Copy link
Author
Sh3Rm4n commented Oct 11, 2020

Ok, I'm failing to adjust the test to pass.

Optimally, this should fullfill following testcases

  1. When ~.vim exists and in runtimepath (which it is for vim)
  2. When neovim-0.4.0, which has stdpath()
  3. When no matching file is in the runtimepath (which is the case for nvim-0.2.0, as it does not have stdpath)

I'll look into that later today.

Is it even possible to create directories in the Dockerfile?

@kkoomen
Copy link
Owner
kkoomen commented Oct 11, 2020

@Sh3Rm4n Just let me do the tests. I'm quick with it. I'll merge this later.

@kkoomen
Copy link
Owner
kkoomen commented Oct 12, 2020 •
edited
Loading

@Sh3Rm4n

Optimally, this should fullfill following testcases

1. When `~.vim` exists and in runtimepath (which it is for vim)

~/.vim also exists for neovim so I assume no need to change this as this is already being checked in the current test, right?

2. When neovim-0.4.0, which has `stdpath()`

What exactly do you want to test here?

3. When no matching file is in the runtimepath (which is the case for nvim-0.2.0, as it does not have stdpath)

When no matching file is in the runpath ... then what?

@Sh3Rm4n
Copy link
Author
Sh3Rm4n commented Oct 13, 2020 •
edited
Loading

Sorry for not being clear about what I want to test.
First and foremost I want the current test to pass with this change.

~/.vim also exists for neovim so I assume no need to change this as this is already being checked in the current test, right?

With the isdirectory check, a buffer with ~/.vim/after/ftplugin/<...>.vim as path is no longer created,
if this path is not in runtimepath. This is the case for neovim, as it does not include .vim in the runtimepath as I've checked with nvim -u NONE +"echo &rtp".

So as this conditional is no longer executed:

vim-doge/autoload/doge/pattern.vim

Line 187 in 9604d8b

if isdirectory(expand(l:p)) && match(&runtimepath, expand(l:p)) >= 0

it will hit this conditional

vim-doge/autoload/doge/pattern.vim

Line 192 in 9604d8b

if has('nvim') && exists('*stdpath') && empty(l:path)

And for nvim-0.2.x, as neovim got the stdpath function in neovim 0.3.0, it will hit this path:

vim-doge/autoload/doge/pattern.vim

Line 205 in 9604d8b

call execute(&showtabline ? 'tabnew' : 'new', 'silent!')

When no matching file is in the runpath ... then what?

But there could also be the case, that any (n)vim session with no matching runtimepath exists, e.g. .vim and ~/vimfiles are removed from the rtp of a vim instance.

In conclusion, with this change we are forced to test all these paths, so that the CI is passing for vim7, vim8, nvim-0.2 and nvim-0.4.

I hope I made it a bit more clear now.

@kkoomen
Copy link
Owner
kkoomen commented Oct 14, 2020

@Sh3Rm4n Well, since you seem to know the ins and outs, I suggest you write the tests. Mention me when you're done and I'll review it again.

@Sh3Rm4n
Copy link
Author
Sh3Rm4n commented Oct 14, 2020

Before I go about writing the tests, is it possible to create directories or files in the CI container?

https://travis-ci.com/github/kkoomen/vim-doge/jobs/397935105#L268

This is where I gave up, because I do not know how to test it otherwise.

@kkoomen
Copy link
Owner
kkoomen commented Oct 14, 2020 •
edited
Loading

@Sh3Rm4n No need to create those directories . When we create a doc standard we open a new file with a given path but don't save it. The user can choose to save it. If those directories don't exists then the user should create it first, vim-doge is not responsible whether those directories exists or not, hence why we check the current buffer's path via AssertEqual expand('~/.vim/after/ftplugin/foo.vim'), expand('%:p')

You can remove all the call mkdir() and execute "!rmdir" statements.

Also, use more modern syntax such as using execute() as a function instead of execute as a command and pay attention to the repository's standards by adding a single white-space before and after a dot operator ..

@Sh3Rm4n Sh3Rm4n force-pushed the master branch 8 times, most recently from 4802770 to e6f4647 Compare October 18, 2020 14:25
@Sh3Rm4n
Copy link
Author
Sh3Rm4n commented Oct 18, 2020

Should be ready for review.

@kkoomen kkoomen self-requested a review October 19, 2020 01:11
@kkoomen kkoomen closed this Oct 23, 2020
@kkoomen
Copy link
Owner
kkoomen commented Oct 23, 2020 •
edited
Loading

@Sh3Rm4n Sorry for force-pushing and screwing up the PR. I can't (force) push to your fork. Can you recreate the fork based off the new master branch and then download the following vim-doge.patch to re-add your functionality:

vim-doge.patch 
diff --git a/autoload/doge/pattern.vim b/autoload/doge/pattern.vim
index 510b0f5..282590b 100644
--- a/autoload/doge/pattern.vim
+++ b/autoload/doge/pattern.vim
@@ -184,17 +184,20 @@ function! doge#pattern#custom(name) abort
   " otherwise create a new file with an appropriate path.
   let l:path = ''
   for l:p in ['~/.vim', '~/vimfiles']
-    if isdirectory(expand(l:p))
-      let l:path = expand(l:p)
+    let l:p = expand(l:p)
+    if match(&runtimepath, l:p) >= 0
+      let l:path = l:p
       break
     endif
   endfor
-  if has('nvim') && empty(l:path)
-    let l:path = stdpath('config')
-  endif
-  if !empty(l:path)
-    let l:path .= '/after/ftplugin/' . l:this_ft . '.vim'
+  if empty(l:path)
+    if exists('*stdpath')
+      let l:path = stdpath('config')
+    else
+      let l:path = getcwd()
+    endif
   endif
+  let l:path .= '/after/ftplugin/' . l:this_ft . '.vim'
   if filereadable(l:path)
     let l:cmd = &showtabline ? 'tabedit' : 'split'
     call execute(l:cmd . fnameescape(l:path), 'silent!')
@@ -204,9 +207,7 @@ function! doge#pattern#custom(name) abort
   else
     call execute(&showtabline ? 'tabnew' : 'new', 'silent!')
     setfiletype vim
-    if !empty(l:path)
-      call execute('file ' . fnameescape(l:path), 'silent!')
-    endif
+    call execute('file ' . fnameescape(l:path), 'silent!')
   endif
 
   " Generate the template and paste it at the top of the file.
diff --git a/test/commands/create-doc-standard.vader b/test/commands/create-doc-standard.vader
index 0b8cf12..263210a 100644
--- a/test/commands/create-doc-standard.vader
+++ b/test/commands/create-doc-standard.vader
@@ -5,6 +5,9 @@
 # ==============================================================================
 
 Execute (Create a new doc standard based on phpdoc, setup dummy pattern and execute the DogeCreateDocStandard command):
+  if !has('nvim')
+    let &runtimepath .= ',' . expand('~/.vim')
+  endif
   enew
   setfiletype php
   let b:doge_patterns = {
@@ -48,7 +51,15 @@ Execute (Create a new doc standard based on phpdoc, setup dummy pattern and exec
   DogeCreateDocStandard phpdoc
 
 Then (Expect a file to be created with specific content):
-  AssertEqual expand('~/.vim/after/ftplugin/php.vim'), expand('%:p')
+  let b:path = '/after/ftplugin/php.vim'
+  if !has('nvim')
+    let b:path = '~/.vim' . b:path
+  elseif exists('*stdpath')
+    let b:path = '~/.config/nvim' . b:path
+  else
+    let b:path = getcwd() . b:path
+  endif
+  AssertEqual expand(b:path), expand('%:p')
 
   let b:content = join([
   \  '" Preserve existing doge settings.',
@@ -81,16 +92,27 @@ Then (Expect a file to be created with specific content):
   AssertEqual b:content, join(getline(line('^'), line('$')), "\n")
   unlet b:content
 # Remove the buffer to ensure every test will create a new doc standard file.
-  call delete(expand('~/.vim/after/ftplugin/php.vim'))
+  call delete(expand(b:path))
   bdelete!
 
 Execute (Create a new doc standard for an unsupported filetype, setup dummy pattern and execute the DogeCreateDocStandard command):
+  if !has('nvim')
+    let &runtimepath .= ',' . expand('~/.vim')
+  endif
   enew
   setfiletype foo
   DogeCreateDocStandard foo
 
 Then (Expect a file to be created with specific content):
-  AssertEqual expand('~/.vim/after/ftplugin/foo.vim'), expand('%:p')
+  let b:path = '/after/ftplugin/foo.vim'
+  if !has('nvim')
+    let b:path = '~/.vim' . b:path
+  elseif exists('*stdpath')
+    let b:path = '~/.config/nvim' . b:path
+  else
+    let b:path = getcwd() . b:path
+  endif
+  AssertEqual expand(b:path), expand('%:p')
 
   let b:content = join([
   \  '" Preserve existing doge settings.',
@@ -123,5 +145,5 @@ Then (Expect a file to be created with specific content):
   AssertEqual b:content, join(getline(line('^'), line('$')), "\n")
   unlet b:content
 # Remove the buffer to ensure every test will create a new doc standard file.
-  call delete(expand('~/.vim/after/ftplugin/foo.vim'))
+  call delete(expand(b:path))
   bdelete!

Then after downloading it, run: patch -p1 < vim-doge.patch, commit it and re-open another PR.

EDIT: nevermind, I'll add a new PR myself, see #116

kkoomen added a commit that referenced this pull request Oct 24, 2020
@kkoomen
feat: recreate PR #104
9c6e84c
@kkoomen kkoomen mentioned this pull request Oct 24, 2020
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kkoomen kkoomen Awaiting requested review from kkoomen

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
@Sh3Rm4n @kkoomen
0