onivim / oni

Oni: Modern Modal Editing - powered by Neovim
https://www.onivim.io
MIT License
11.34k stars 298 forks source link

Lint errors moving on scroll #839

Closed akinsho closed 6 years ago

akinsho commented 7 years ago

Firstly thanks for the amazing project!! :thumbsup:

I'm really looking forward switching to oni as my daily editor.

At the moment though I having some trouble with the linter plugin, which may be a known bug, but I couldn't find reference to it anywhere.

What the screen shot attempts to illustrate is that when the linter runs on save errors show up but on scrolling the underline markers move on scroll so they end up in the wrong place. Also it does not seem to be reading my local tsconfig from the project root or my the tsconfig I have in my home directory.

I've tried with and without the separate linter plugin. I've also tried deactivating my init.vim config and using only the default config but this error still occurs

screen shot 2017-10-30 at 14 07 36

Dev tools screenshot

screen shot 2017-10-30 at 14 09 51

I use ale.vim for linting in iTerm and I wonder if it's possible to use this instead if its something thats still in flux, at the moment the plugin doesn't seem to work

bryphe commented 7 years ago

Hi @Akin909 ,

Thanks for the kind words (and your support!). Glad you're on the way to using Oni as your daily editor 😄 And appreciate the detailed issue and screenshots too!

The underline markers moving with the scroll is definitely a bug - it seems we're not picking up the scroll events in all the right places (which would trigger updating the error adorners/underlines). This should've been fixed by #805 but it looks like there are still some cases where we're not updating proplery.

Seems like another issue that it isn't picking up your tsconfig.json - would you be able to share the folder structure / hierarchy of your project? We rely mostly on the TypeScript Standalone Server to pick up the tsconfig.json - but there might be cases especially with a hierarchy of tsconfig.json where it needs help on our end. If it happens to be an OSS project I can clone / reproduce the issue that would be awesome!

I use ale.vim for linting in iTerm and I wonder if it's possible to use this instead if its something thats still in flux, at the moment the plugin doesn't seem to work

Should definitely be possible. You can try these steps here to use ale.vim from within Oni. As an aside, I'm working on a big refactor of our language services (#819) - that will make it easier to switch to disable the existing language services, or switch to alternate strategies (like use flow for typescript, or an alternate TypeScript language server, etc). LMK how it goes or if you hit any issues there!

akinsho commented 7 years ago

Thanks for the reply, Re the TS config unfortunately its a work repo, although if it helps its pretty deeply nested but the tsconfig is at the root of the project. I had a go with a much much smaller project (don't think it'll be much help) with a tsconfig and It also seems not to be picking up the types for the project.

Also thanks for the pointer re installing ale in the oni plugins directory i'll give it a go. Out of curiousity I have some plugins which work in Oni out of the box (like easy motion) without needing to be in the oni plugins dir, when does a plugin need to go in there or is the practice with oni that all plugins should really go in there?

bryphe commented 7 years ago

Sure thing!

Thanks for the additional info. I tested a bit and did find an issue with release builds not getting the full benefit of TypeScript language services - TypeScript relies on several libraries to define the environment, and those weren't included in the distribution builds. I'm working on a PR for that right now + test case.

Out of curiousity I have some plugins which work in Oni out of the box (like easy motion) without needing to be in the oni plugins dir, when does a plugin need to go in there or is the practice with oni that all plugins should really go in there?

It sounds like you have Oni set up to use your init.vim, and you already have a plugin manager set up? If that's the case, you don't really need to worry about adding to the oni plugins directory. Oni also has it's own plugin API - plugins using that would need to go in oni's plugin directory. But in terms of vim plugins, it's up to you - for things like easy motion, you probably want it both if you're using Oni and terminal neovim, so that would be a reason to use Vim's plugin manager for it.

My goal though is to make it easy for newcomers to use both Vim and Oni plugins - so someone new to Oni/modal editing doesn't need to know how to set up an init.vim and pick out a plugin manager - they can use an out-of-the-box plugin manager (like the experience in Atom or VSCode). Not here yet though 😄

akinsho commented 7 years ago

Yes I use vim plug to manage my plugins and deoplete for autocompletion, ale for linting (plus dozens more), at the moment though ale which usually shows lint errors with signs in the signcolumn (or gitgutter which shows git changed lines ) aren't working is there a specific config for allowing the sign column? sorry feel like I'm a little of the original topic.

bryphe commented 7 years ago

at the moment though ale which usually shows lint errors with signs in the signcolumn (or gitgutter which shows git changed lines ) aren't working is there a specific config for allowing the sign column?

There shouldn't be anything in Oni stopping this - I just ran this VimL as a litmus test (from http://vimdoc.sourceforge.net/htmldoc/sign.html)

:highlight SignColumn guibg=darkgrey
:sign define piet text=>> texthl=search
:exe ":sign place 2 line=23 name=piet file=" . expand("%:p")

And I do see the sign show up as expected: image

I wonder if there is some other interaction occurring with Oni that is preventing this from working?

I also tested just vim-gitgutter: image

And it seemed to work OK on my machine.

A few things / questions to dig further:

sorry feel like I'm a little of the original topic.

No worries! Sorry you're hitting these issues. It'll be helpful for others if they search for ale or gitgutter anyway 😄

akinsho commented 7 years ago

I use quite a few, so I tried a minimal vimrc

" Plug Setup ===================== {{{
"This will autoinstall vim plug if not already installed
if empty(glob('~/.vim/autoload/plug.vim'))
  silent !curl -fLo ~/.vim/autoload/plug.vim --create-dirs
        \ https://raw.githubusercontent.com/junegunn/vim-plug/master/plug.vim
  augroup VimPlug
    au!
    autocmd VimEnter * PlugInstall --sync | source $MYVIMRC
  augroup END
endif

call plug#begin('~/.vim/plugged')
Plug 'airblade/vim-gitgutter'
call plug#end()

Which unfortunately still didn't work, I also ran the commands above and that worked so it seems the signcolumn works. Lastly I ran ALEInfo which show a status log etc

screen shot 2017-11-06 at 22 04 14

None of my executables were found by Ale which makes me wonder if git can be found by gitgutter although when I run a terminal with my full config it has access to fzf and git for example.

bryphe commented 7 years ago

None of my executables were found by Ale which makes me wonder if git can be found by gitgutter although when I run a terminal with my full config it has access to fzf and git for example.

Ah interesting, that's a good find. It's possible those binaries aren't being found - and given gitgutter isn't working, it seems likely that the git executable isn't working. Which platform are you running on?

Running !git --version in Oni - that would be a good litmus test to see if it is finding that binary or not.

Also, checking the PATHs that Oni knows about would be interesting - this can be done by checking the value of process.env.PATH in the debug console (Control+Shift+P -> Open Developer Tools).

akinsho commented 7 years ago

Just tried git version and it's 2.13.6 (Apple Git-96) in Oni and in my shell its 2.15.0(I also use hub which is a wrapper around git, not sure if that'd have and impact. The process.env.PATH that Oni detects is not the path variable I export from my zshrc

screen shot 2017-11-06 at 23 37 31

Should I be setting this in the Oni config - Just read the docs on this and saw the section on setting this. I wonder if theres a wayI can adjust it so Oni picks it up? I tried passing it to my config but that didn't change oni's PATH.

bryphe commented 7 years ago

Ah good catch, thanks for testing that out.

Yes, depending on how it's run, it might not have the same $PATH variable as in your shell (especially if you run it from the dock or finder).

You can set the environment.additionalPaths configuration value to make sure Oni is always passing those down. Let me know if that works for you.

akinsho commented 7 years ago

I had a go with the additional paths but the path oni is using didn't update at all, and the default vars given to the additional paths variable seem like they should include usr/local/bin already (which I believe would resolve my issue and is what I was trying to add), I ran into a similar issue with vimr and the solution i found turned out to be running an interactive shell I have no idea if that would relate to this at all but the problem was with finding go executables

akinsho commented 7 years ago

@bryphe I've had a look at the pre-release version and its soo much better 👍 🎆 thanks !! regarding the path issue i've been looking into that and noted #708 relates to this as well(closed), I think NVIM now correctly sources the relevant rc file and gets the PATH, as in the nvim terminal in oni (release version) I get a different path to echo $PATH in nvim (via nvim commandline in oni). I had a read through the code and I'm not sure when the cli/oni file is run but its one of the only points when the $PATH is set. I think the additional paths aren't passed through or rather never used in the build versions as when running it locally via the cli it all works with a full path.

I've found this module which hopefully addresses paths not being passed in gui apps on Mac OS (which is where I'm running this sorry for the late info) .

I think my initial issue re the error highlighting is resolved and I'm getting correct linting errors now from the tslint and tss plugins so I'm happy to close it thanks for the fix. Re. the $PATH i'd be happy to have a go at looking into this although I currently cant figure out how to make changes then build and install the electron app since if I run it via the CLI it all works fine anyway. PS. also happy to leave it be and stop pestering

bryphe commented 7 years ago

@Akin909 glad to hear it's working better! 👍

Regarding the path issue - I believe you're right, it seems like we aren't actually passing the additional paths down to the Neovim process.

This is the code we use to spin up the neovim process: https://github.com/onivim/oni/blob/c4b3ce408ca5f9b377f034840625bdea6b0d347f/browser/src/neovim/NeovimProcessSpawner.ts#L54

But we're not actually passing down the additional paths variable, it seems. We have a helper method to do that over here: https://github.com/onivim/oni/blob/c4b3ce408ca5f9b377f034840625bdea6b0d347f/browser/src/Plugins/Api/Process.ts#L10

So it might just be if we hook that up, Neovim will get the $PATH that it needs.

The module you pointed out is interesting too - maybe that could keep us from needing the environment.additionalPaths config at all?

akinsho commented 7 years ago

I can look into this and have a go at replacing the cp.spawn with the helper function, and try the shell-env module to see if this loads in the full path, I think it would possibly remove the need for the additional paths although I think if a user wanted to add something to the path in oni but not in their shell for some unfathomable reason it might be good to keep the config option around?

How do I build and create the electron app so I can test it outside of the cli environment?

bryphe commented 7 years ago

I can look into this and have a go at replacing the cp.spawn with the helper function, and try the shell-env module to see if this loads in the full path

That would be awesome! Appreciate the help!

We have a link on the Wiki for building locally - Development - let me know if you hit any hurdles

akinsho commented 7 years ago

I've had replaced the call to child_process with a call to the helper function and it runs correctly locally although having read the development i'm still not entirely clear on how to build the project and output an version of the app to run outside of the console.

Regarding shell-env I tried adding it to the app and it seems to mess with the types in the app and it fails to build. There isn't a type package for it on definitely-typed. Im attempting to add type definitions for it manually, in the definitions folder

akinsho commented 6 years ago

@bryphe this issue is much improved and I'm thinking i'd like to close this though there are 2 tangential issues which persist

  1. one is that some times on changing branch or a change that impacts the whole buffer like autoformatting, sorry I can try and narrow this down, a single error marker shows up at the end of the buffer sometimes reporting a multiple exports issue (usually because I have my default exports at the bottom of the file though there aren't multiple.
  2. Another issue is that errors are sometimes slow to disappear you can tell that they aren't real as there in no diagnostic message and also I use this in tandem with ale and it's markers disappear.
screen shot 2018-01-09 at 14 32 21

Not sure these should be raised as separate issues or discussed here?

akinsho commented 6 years ago

Closing as I think a lot has changed and this issue is a bit dated