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

Show tsserver hints/suggestions in Ale. #3362

Merged
merged 4 commits into from
Jan 20, 2021
Merged

Show tsserver hints/suggestions in Ale. #3362

merged 4 commits into from
Jan 20, 2021

Conversation

daliusd
Copy link
Contributor

@daliusd daliusd commented Sep 23, 2020

No description provided.

@evmorov
Copy link

evmorov commented Sep 23, 2020

The documentation says:

Populating the |loclist| with warning and errors

and

the |loclist| will be populated with any warnings and errors which are found by ALE

tsserver hints/suggestions are infos. Is it a bug that the loclist is populated with them? Personally, I don't like it. VSCode and IDEA don't populate "Problems" with hints either:

image

@evmorov
Copy link

evmorov commented Sep 23, 2020

Is it possible to make a setting for that? coc.vim and vscode has typescript.suggestionActions.enabled.

@daliusd
Copy link
Contributor Author

daliusd commented Sep 23, 2020

Interestingly info is expected in loclist, e.g. ALENext has -info flag. From documentation:

  `-error`, `-warning` and `-info` enable jumping to errors, warnings or infos
    respectively, ignoring anything else. They are mutually exclusive and if
    several are provided the priority is the following: error > warning > info.

So question is what setting is for? So you can use -error and warning flags if info does not bother you.

@evmorov
Copy link

evmorov commented Sep 23, 2020

e.g. ALENext has -info

I'm not using ALENext. I have let g:ale_open_list = 1. In my workflow if there is something in the loclist then it must be fixed.

So question is what setting is for?

To turn off tsserver hints.

P.S. I'm just a user of ALE. Possible that my opinion is wrong

@daliusd
Copy link
Contributor Author

daliusd commented Sep 23, 2020

I'm only user as well 😄. One idea: g:ale_lsp_show_message_severity should be respected by tsserver as well. That would solve this problem. However I'm not sure if I'm right or wrong here. Let's wait for more input from core developers (e.g. @w0rp)

@evmorov
Copy link

evmorov commented Sep 23, 2020

Nevermind. I've started to use ALENext and set g:ale_open_list to 0. I'll continue using your branch until this PR is merged.

@stale
Copy link

stale bot commented Oct 22, 2020

This pull request has been automatically marked as stale because it has not been updated recently. Make sure to write tests and document your changes. See :help ale-dev for information on writing tests.
If your pull request is good to merge, bother w0rp or another maintainer again, and get them to merge it.

@stale stale bot added the stale PRs/Issues no longer valid label Oct 22, 2020
@daliusd
Copy link
Contributor Author

daliusd commented Oct 22, 2020

@w0rp any comments?

@stale stale bot removed the stale PRs/Issues no longer valid label Oct 22, 2020
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.

This is almost good. Just make sure that the items use the type 'I' for "info", and that we have a setting to ignore suggestions. I think it's worth turning them on by default, so people are aware they exist, and letting people turn suggestions off with one setting. Let's call this setting ale_lsp_suggestions, set it to 1 by default, and document that setting it to 0 will disable suggestions for tsserver and from LSP servers.

We could add ale_minimum_severity at some point for everything, but I think it's worth being able to control specifically suggestions from tsserver and LSP servers as a separate setting.

@w0rp w0rp added this to In Review in On the Radar via automation Oct 22, 2020
@w0rp w0rp self-assigned this Oct 22, 2020
@daliusd
Copy link
Contributor Author

daliusd commented Oct 23, 2020

Sorry, I will not be able to fix it completely as I don't see using tsserver with Ale myself in the future.

@daliusd
Copy link
Contributor Author

daliusd commented Oct 25, 2020

Update:

Items use type 'I' for suggestions. It is set in ale/autoload/ale/lsp/response.vim line 109:

        if get(l:diagnostic, 'category') is# 'suggestion'
            let l:loclist_item.type = 'I'
        endif

I'm working on setting now.

@daliusd daliusd requested a review from w0rp October 25, 2020 21:45
@daliusd
Copy link
Contributor Author

daliusd commented Oct 30, 2020

@w0rp please review again

@daliusd
Copy link
Contributor Author

daliusd commented Nov 14, 2020

@w0rp please merge this one as well. I personally find it useful.

@stale
Copy link

stale bot commented Dec 19, 2020

This pull request has been automatically marked as stale because it has not been updated recently. Make sure to write tests and document your changes. See :help ale-dev for information on writing tests.
If your pull request is good to merge, bother w0rp or another maintainer again, and get them to merge it.

@stale stale bot added the stale PRs/Issues no longer valid label Dec 19, 2020
@daliusd
Copy link
Contributor Author

daliusd commented Dec 20, 2020

@w0rp 😉

@stale stale bot removed the stale PRs/Issues no longer valid label Dec 20, 2020
@stale
Copy link

stale bot commented Jan 17, 2021

This pull request has been automatically marked as stale because it has not been updated recently. Make sure to write tests and document your changes. See :help ale-dev for information on writing tests.
If your pull request is good to merge, bother w0rp or another maintainer again, and get them to merge it.

@stale stale bot added the stale PRs/Issues no longer valid label Jan 17, 2021
Copy link
Contributor

@hsanson hsanson 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 the patience. This looks good to me.

@stale stale bot removed the stale PRs/Issues no longer valid label Jan 20, 2021
@hsanson hsanson merged commit 783cf4a into dense-analysis:master Jan 20, 2021
On the Radar automation moved this from In Review to Done Jan 20, 2021
@daliusd
Copy link
Contributor Author

daliusd commented Jan 20, 2021

🍻

@daliusd daliusd deleted the tsserver_hints branch January 20, 2021 11:47
@muzuiget
Copy link

muzuiget commented Feb 6, 2021

Finally found this issue, this change confuse me.

Usually I get same result from command tsc and tsserver in Ale, so I can lint my code without open it in Vim.

After this change, when I run tsc, no warnings/errors, but open it in Vim, some warnings/errors exits, eg:

6133: 'url' is declared but its value is never read.

This message is look like it lint by tsc --noUnusedParameters or unusedParameters=true in tsconfig.json, it looks like tsc and tsserver treat tsconfig.json in difference behavior. It takes some time to find this issue.

After reading above comments, set ale_lsp_suggestions=1 seen to solve my problem.

@w0rp
Copy link
Member

w0rp commented Feb 6, 2021

I think you won't be alone in finding that confusing. I'll turn them off by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants