Skip to content
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

Use relative paths when previewing file locations #2238

Merged
merged 13 commits into from
Jan 27, 2019
Merged

Use relative paths when previewing file locations #2238

merged 13 commits into from
Jan 27, 2019

Conversation

chaucerbao
Copy link
Contributor

Absolute paths can be long and repetitive, and cause the more interesting details (in a file location preview) to be truncated. Instead, display file locations relative to the current working directory.

@chaucerbao
Copy link
Contributor Author

Linter is failing with:

autoload/ale/preview.vim:54 Do not use getcwd(), as it could run from the wrong buffer. Use expand('#' . a:buffer . ':p:h') instead.

But we (probably) don't want the results relative to specific buffer; we want it relative to the current working directory, which is precisely what getcwd() provides.

@w0rp is there a better way around this?

Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

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

Use a " no-custom-checks comment at the end of line to ignore the warning about getcwd().

These changes completely replace absolute paths with relative paths. Instead of doing that, add options to the commands for showing relative paths, like :ALEExampleCommand -relative, and show absolute paths by default.

Copy link
Contributor Author

@chaucerbao chaucerbao left a comment

Choose a reason for hiding this comment

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

I added an optional use_relative_paths flag to the ale#preview#ShowSelection() function to enable the display of relative paths, defaulting to absolute paths.

" Absolute paths
ale#preview#ShowSelection([])
ale#preview#ShowSelection([], { 'use_relative_paths': 0 })

" Relative paths
ale#preview#ShowSelection([], { 'use_relative_paths': 1 })

But I'm not familiar with how to parse a -relative option from an ex-command, and then, the best way to pass that through multiple function calls to reach ale#preview#ShowSelection()

  1. ale#references#Find()
  2. s:FindReferences()
  3. s:OnReady
    1. ale#references#HandleTSServerResponse
    2. ale#references#HandleLSPResponse
  4. ale#preview#ShowSelection

Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

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

See :help command. You can pass <args> or similar to a function and parse the string for options. You can access the raw string for the options and parse it.

endif
endfor
endif

for l:linter in ale#linter#Get(&filetype)
if !empty(l:linter.lsp)
call s:FindReferences(l:linter)
Copy link
Member

Choose a reason for hiding this comment

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

Instead of modifying a script variable, pass the options in here. Forward that on to the OnReady function. You can then save the options in s:references_map. You can write let l:options = remove(s:references_map, a:response.id) above to get the options out at the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing me in the right direction. I applied your solution and added tests.

Lemme know if there's anything else I can do to clean up (or correct) the code.

@w0rp w0rp merged commit 6288c8b into dense-analysis:master Jan 27, 2019
@w0rp
Copy link
Member

w0rp commented Jan 27, 2019

Cheers! 🍻

@chaucerbao chaucerbao deleted the feature/lsp-relative-path branch January 28, 2019 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants