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

Add -relative option to ALESymbolSearch #2255

Merged
merged 5 commits into from
Feb 8, 2019
Merged

Add -relative option to ALESymbolSearch #2255

merged 5 commits into from
Feb 8, 2019

Conversation

chaucerbao
Copy link
Contributor

I added the -relative option to ALESymbolSearch, and the associated test.

However, I only use TSServer, which doesn't seem to be implemented for ALESymbolSearch, so I'm unable to actually test the -relative option IRL. I kept changes minimal, and the existing tests pass. But I would certainly prefer a quick real-world test to confirm.

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.

Thanks for working on this too. 👍 I wouldn't worry about tsserver just yet. I might have a go at implementing symbol search for tsserver later. See my comments here.

I think this is generally good, so you can update the documentation for the command to mention the new switch.

autoload/ale/symbol.vim Outdated Show resolved Hide resolved
autoload/ale/symbol.vim Outdated Show resolved Hide resolved
@chaucerbao
Copy link
Contributor Author

@w0rp What's the best way to request another review after change requests are addressed?

@w0rp
Copy link
Member

w0rp commented Feb 7, 2019

Check out 19cc724 and examples in test/test_parse_command_args.vader. I have now added a function for parsing command arguments we can use everywhere.

@w0rp
Copy link
Member

w0rp commented Feb 7, 2019

If you wait, I'll look at the pull request eventually.

@chaucerbao
Copy link
Contributor Author

chaucerbao commented Feb 7, 2019

Gotcha, didn't mean to nag. It just isn't clear what state the PR is in after a change request since it stays as "Changes Requested". More of a GitHub issue, I suppose.

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.

Perfect! 👌

@w0rp w0rp merged commit 1fb0de2 into dense-analysis:master Feb 8, 2019
@chaucerbao chaucerbao deleted the feature/add-relative-paths-to-lsp-symbol-search branch February 8, 2019 20:45
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