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

migrate: from efm to null-ls ( #500 #504 ) #513

Merged
merged 27 commits into from
Feb 18, 2023
Merged

migrate: from efm to null-ls ( #500 #504 ) #513

merged 27 commits into from
Feb 18, 2023

Conversation

CharlesChiuGit
Copy link
Collaborator

@CharlesChiuGit CharlesChiuGit commented Feb 13, 2023

WIP

@CharlesChiuGit CharlesChiuGit changed the title migrate: from efm to null-ls migrate: from efm to null-ls ( #504 ) Feb 13, 2023
@CharlesChiuGit
Copy link
Collaborator Author

CharlesChiuGit commented Feb 13, 2023

Do we need to re-implement FormatToggle autocmd?
Since we only use null-ls for formatting now, it would be much easier if there's some way to disable null-ls temporally.

@CharlesChiuGit CharlesChiuGit changed the title migrate: from efm to null-ls ( #504 ) migrate: from efm to null-ls ( #500 #504 ) Feb 13, 2023
@CharlesChiuGit CharlesChiuGit changed the title migrate: from efm to null-ls ( #500 #504 ) migrate: from efm to null-ls ( #500 #504 ) Feb 13, 2023
@CharlesChiuGit
Copy link
Collaborator Author

CharlesChiuGit commented Feb 14, 2023

@aarnphm PTAL

I think it's better to config sources in null-ls since some formatters can also be used as lsp, like markdownlint.
I'm not sure what's being set via mason-null-ls' automatic_setup, so I think it's better to setup the sources by ourselves separately.

@CharlesChiuGit CharlesChiuGit marked this pull request as draft February 14, 2023 07:24
@CharlesChiuGit CharlesChiuGit marked this pull request as ready for review February 14, 2023 07:24
Copy link
Collaborator

@aarnphm aarnphm left a comment

Choose a reason for hiding this comment

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

LGTM. cc @Jint-lzxy when you have bandwidth.

Copy link
Owner

@ayamir ayamir left a comment

Choose a reason for hiding this comment

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

:FormatToggle and :FormatterToggle haven't been implemented.

@CharlesChiuGit
Copy link
Collaborator Author

@ayamir Have we tried https://github.com/lukas-reineke/lsp-format.nvim before?
It seems pretty handy.

@CharlesChiuGit
Copy link
Collaborator Author

It seems like the old formmatting.lua is more powerful than ls-format.nvim. I think I'll tack it back and modify a bit.

@ayamir
Copy link
Owner

ayamir commented Feb 16, 2023

Yes, but we chose to realize the functions by ourselves later.

@CharlesChiuGit
Copy link
Collaborator Author

CharlesChiuGit commented Feb 16, 2023

Do we need to keep the choice to use the actual LSP's formatting ability instead of only use null-ls from now on?
From a end-user view point, I think it's much easier to only set formatters in null-ls config rather than modify formatting.lua.

How do u guys think?

@CharlesChiuGit
Copy link
Collaborator Author

CharlesChiuGit commented Feb 16, 2023

In my experiment, block_list doesn't seem valid. How do I block a filetype for formatting?(it doesn't seem to be included in the wiki?

@Jint-lzxy
Copy link
Collaborator

In my experiment, block_list doesn't seem valid. How do I block a filetype for formatting?(it doesn't seem to be included in the wiki?

@CharlesChiuGit Yep I plan to review & reimplement :FormatToggle and :FormatterToggle by the end of this week 👍 Some interaction logic has changed during this migration.

Do we need to keep the choice to use the actual LSP's formatting ability instead of only use null-ls from now on? From a end-user view point, I think it's much easier to only set formatters in null-ls config rather than modify formatting.lua.

How do u guys think?

Not sure if I understood this question correctly, but IMHO this choice should be left to users. Some individuals' / organizations' projects would prefer to have formatting support provided by the language server (especially if they already have a custom configuration file in use).

FWIW what we can do is remind users of those conflicting format approaches and ask them to confirm their preferences when encountering this for the first time. We can store these results in a cache file, so that the changes made will be persistent. (This is also the feature that will be implemented when refactoring :FormatterToggle)

@CharlesChiuGit
Copy link
Collaborator Author

@Jint-lzxy Then i'll just change efm to null-ls and leave everything else untouched. @ayamir how do u think?

@ayamir
Copy link
Owner

ayamir commented Feb 16, 2023

Agree.

@CharlesChiuGit
Copy link
Collaborator Author

:FormatToggle and :FormatterToggle haven't been implemented.

@ayamir PTAL

@CharlesChiuGit
Copy link
Collaborator Author

CharlesChiuGit commented Feb 16, 2023

side note: I think some of formatting.lua's setting should be port to core/settings.lua. But maybe @Jint-lzxy has already take that into consideration lol

@CharlesChiuGit
Copy link
Collaborator Author

CharlesChiuGit commented Feb 16, 2023

2ff5d96 (#513) moves lsp and null-ls sources tables to core/settings.lua. Will add this to wiki once merged.

Copy link
Owner

@ayamir ayamir left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@aarnphm
Copy link
Collaborator

aarnphm commented Feb 17, 2023

Feel free to merge it in when it is gtg 😄

@CharlesChiuGit
Copy link
Collaborator Author

CharlesChiuGit commented Feb 17, 2023

I think maybe @Jint-lzxy is reviewing this ATM?

@CharlesChiuGit CharlesChiuGit removed the request for review from Jint-lzxy February 18, 2023 04:07
@CharlesChiuGit CharlesChiuGit merged commit 9323316 into ayamir:main Feb 18, 2023
@CharlesChiuGit CharlesChiuGit deleted the migrate/null-ls branch February 18, 2023 04:09
boomker pushed a commit to boomker/uvcode-nvim that referenced this pull request Feb 20, 2023
YanTree pushed a commit to YanTree/nvim that referenced this pull request Apr 9, 2023
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.

4 participants