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 dart analysis server to linter #3069

Merged
merged 7 commits into from
Jan 22, 2021
Merged

Add dart analysis server to linter #3069

merged 7 commits into from
Jan 22, 2021

Conversation

nelsyeung
Copy link
Contributor

@nelsyeung nelsyeung commented Mar 22, 2020

Add Dart analysis_server to linter. Closes #1005

guildem
guildem previously approved these changes Apr 23, 2020
RRethy
RRethy previously approved these changes Jun 5, 2020
Copy link

@RRethy RRethy left a comment

Choose a reason for hiding this comment

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

I've been using it locally and it's working well for me.

@bluz71
Copy link

bluz71 commented Jun 9, 2020

I just tested this PR locally.

Functionally it works great.

However, I am receiving the following noise in the command line area:

Dart_Analysis_Server

Unknown method textDocument/didSave

This appears every time I hit save, which is my trigger for a lint to occur.

I am using Flutter v1.12.13+hotfix.8.

It would be highly desirable to silently consume this particular message, it gets annoying fast.

Best regards.

@RRethy
Copy link

RRethy commented Jun 9, 2020

I'm also seeing that, part of my config was hiding it so I didn't notice previously.

@RRethy
Copy link

RRethy commented Jun 9, 2020

sublimelsp/LSP#550 this issue describes it.

@bluz71
Copy link

bluz71 commented Jun 10, 2020

I'm also seeing that, part of my config was hiding it so I didn't notice previously.

How did you hide it? This PR probably should probably include that workaround.

@RRethy
Copy link

RRethy commented Jun 10, 2020

It's not applicable, it was being hidden because I was echoing a bunch of other messages. It would still show up in :messages but with other messages so I didn't notice it.

@bluz71
Copy link

bluz71 commented Jun 11, 2020

These issues are relevant:

Correct me if I am wrong, but I think ALE may be sending textDocument/didSave to the Dart Analysis Server even after the server says it doesn't support textDocumentSync > save.

I suspect ALE LSP code needs to be refined to not send textDocument/didSave after the server initially indicates it doesn't support that operation.

As it stands at the moment, every Dart file save is triggering this alert which distracts from genuine lint messages.

@nelsyeung
Copy link
Contributor Author

@bluz71 Do you think it's worth raising this as a separate issue and create a fix in a separate PR?

@bluz71
Copy link

bluz71 commented Jun 12, 2020

Yes, it is a separate issue. It should not hold this up.

I was going to create a new issue today, but got distracted with other stuff. I will do so soon.

@bluz71
Copy link

bluz71 commented Jun 14, 2020

New issue, #3207, spawned about the didSave issue.

This PR can be merged independent of that. This looks ready now.

hwasoocho
hwasoocho previously approved these changes Aug 9, 2020
@stale
Copy link

stale bot commented Sep 25, 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 a bot will close automatically label Sep 25, 2020
@nelsyeung
Copy link
Contributor Author

@w0rp This is certainly not stale and would love to be merged asap.

@stale stale bot removed the stale PRs a bot will close automatically label Sep 25, 2020
@bluz71
Copy link

bluz71 commented Sep 25, 2020

Seconded. Looking forward to seeing this merged.

@stale
Copy link

stale bot commented Oct 23, 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 a bot will close automatically label Oct 23, 2020
@nelsyeung
Copy link
Contributor Author

@w0rp bump

@stale stale bot removed the stale PRs a bot will close automatically label Oct 23, 2020
@bluz71
Copy link

bluz71 commented Oct 23, 2020

I agree, not stale.

@stale
Copy link

stale bot commented Dec 2, 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 a bot will close automatically label Dec 2, 2020
@nelsyeung
Copy link
Contributor Author

I'm not gonna lose this battle to a bot!

@stale stale bot removed the stale PRs a bot will close automatically label Dec 2, 2020
@lukaszo
Copy link

lukaszo commented Dec 8, 2020

Hello @nelsyeung

please consider updating your patch with this tiny patch:

11c11
<     return !empty(l:pubspec) ? fnamemodify(l:pubspec, ':h:h') : ''
---
>     return !empty(l:pubspec) ? fnamemodify(l:pubspec, ':h:h') : '.'

I've just changed it to return current dir when pubspec is not available. In this way it will support also standalone files.

@nelsyeung nelsyeung dismissed stale reviews from hwasoocho and RRethy via 7cd916a December 8, 2020 21:17
RRethy
RRethy previously approved these changes Dec 8, 2020
@matsp
Copy link

matsp commented Dec 21, 2020

Anything missing? Out of the box support would be really great 👍

@nelsyeung
Copy link
Contributor Author

Anything missing? Out of the box support would be really great 👍

Nope, just waiting for review and merge.

@matsp
Copy link

matsp commented Dec 21, 2020

@hwasoocho Can you please review this PR?

@matsp
Copy link

matsp commented Dec 25, 2020

@RRethy @guildem Can someone else review this PR?

guildem
guildem previously approved these changes Dec 26, 2020
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.

Missing tests. Check the test/command_callbak folder for examples on how to write tests for command callbacks.

@nelsyeung nelsyeung dismissed stale reviews from guildem and RRethy via d438a6c January 20, 2021 19:32
@nelsyeung
Copy link
Contributor Author

@hsanson Command callback tests added

hsanson
hsanson previously approved these changes Jan 20, 2021
@hsanson
Copy link
Contributor

hsanson commented Jan 22, 2021

ALE migrated from Travis to Github Actions (see #3548) so it is necessary to rebase this PR from latest master branch to trigger the required checks.

# Add dense-analysis as remote upstream on your local fork. Not 
# needed if already added.
git remote add upstream https://github.com/dense-analysis/ale.git

# Sync your local master with upstream master
git checkout master
git fetch upstream master
git merge upstream/master

# Rebase the PR branch with master
git checkout my-branch-name
git rebase master   # Fix any conflicts you may have

# Force push the updated PR branch to origin to trigger the checks
git push -f origin my-branch-name

@nelsyeung
Copy link
Contributor Author

Looks like it's all ready now!

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.

Ready to go, thanks.

@hsanson hsanson merged commit 1b010bb into dense-analysis:master Jan 22, 2021
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.

Add support for Dart analysis server
8 participants