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.

Sign up for GitHub

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

Jump to bottom

plugins/flutter-tools: refactoring #2822

Merged
merged 2 commits into from
Jan 18, 2025
Merged

plugins/flutter-tools: refactoring #2822

merged 2 commits into from
Jan 18, 2025
+152 −112

Conversation

GaetanLepage
Copy link
Member

@GaetanLepage GaetanLepage commented Jan 11, 2025 •
edited
Loading

Add support for flutter-tools.nvim, a plugin to help create flutter apps in Neovim using the native lsp.

Fixes #2042.

@GaetanLepage GaetanLepage requested a review from a team January 11, 2025 20:05
@khaneliman
Copy link
Contributor

?

@traxys
Copy link
Member

traxys commented Jan 12, 2025

It was already implemented in #2728

@khaneliman
Copy link
Contributor

Looks like we had multiple issues for the same plugin.

@khaneliman khaneliman closed this Jan 12, 2025
@khaneliman khaneliman reopened this Jan 12, 2025
@khaneliman
Copy link
Contributor

Unless you want to add maintainer and reword commit for refactoring?

@GaetanLepage
Copy link
Member Author

I don't know how, but I somehow missed our current implementation.

@GaetanLepage GaetanLepage changed the title plugins/flutter-tools: init plugins/flutter-tools: refactoring Jan 12, 2025
@GaetanLepage
Copy link
Member Author

I've rephrased this patch to be a reformatting then.

@traxys
Copy link
Member

traxys commented Jan 12, 2025

Why would a refactoring be mandated? This needs a bit more context then

@GaetanLepage GaetanLepage marked this pull request as draft January 14, 2025 18:00
@GaetanLepage GaetanLepage marked this pull request as ready for review January 14, 2025 22:58
@GaetanLepage GaetanLepage marked this pull request as draft January 14, 2025 22:58
@GaetanLepage GaetanLepage marked this pull request as ready for review January 16, 2025 23:02
MattSturgeon
MattSturgeon reviewed Jan 17, 2025
View reviewed changes
plugins/by-name/flutter-tools/settings-options.nix Show resolved Hide resolved
plugins/by-name/flutter-tools/settings-options.nix
Comment on lines -40 to +46
Integrate with nvim `dap` + install dart code debugger.
Enable `nvim-dap` integration.
Copy link
Member

Choose a reason for hiding this comment

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

Why drop mention of the dart code debugger?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we "install it" though ?

Copy link
Member

Choose a reason for hiding this comment

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

I assumed it was the upstream plugin's wording. So the question isn't whether we install it but whether the plugin does 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think that the plugin does auto-install it.
https://github.com/nvim-flutter/flutter-tools.nvim/blob/234a9d4022d0a17301e85a08660d489bffb7383f/README.md?plain=1#L445-L447

Maybe we should add a warning ?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe. Looking at the upstream README there's several dependencies to consider... Happy for that to be tackled in other PRs though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I already added a warning for plugins.dap.enable actually.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do see a telescope integration upstream, too. We could handle that in another PR.

plugins/by-name/flutter-tools/settings-options.nix Outdated Show resolved Hide resolved
plugins/by-name/flutter-tools/settings-options.nix Outdated Show resolved Hide resolved
plugins/by-name/flutter-tools/settings-options.nix Outdated Show resolved Hide resolved
plugins/by-name/flutter-tools/settings-options.nix Outdated Show resolved Hide resolved
plugins/by-name/flutter-tools/settings-options.nix Show resolved Hide resolved
plugins/by-name/flutter-tools/settings-options.nix Show resolved Hide resolved
plugins/by-name/flutter-tools/settings-options.nix Outdated Show resolved Hide resolved
plugins/by-name/flutter-tools/settings-options.nix Outdated Show resolved Hide resolved
@GaetanLepage GaetanLepage force-pushed the flutter branch 2 times, most recently from 7cba04d to d92e32b Compare January 17, 2025 08:50
@GaetanLepage GaetanLepage force-pushed the flutter branch 2 times, most recently from 057976c to 000f1da Compare January 17, 2025 13:32
khaneliman
khaneliman approved these changes Jan 18, 2025
View reviewed changes
Copy link
Contributor

@khaneliman khaneliman left a comment

Choose a reason for hiding this comment

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

LGTM from the conversations I've read about why changes were being made.

@GaetanLepage
Copy link
Member Author

@mergify queue

@mergify Mergify
Copy link
Contributor

mergify bot commented Jan 18, 2025 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at cbf960e

@mergify Mergify
Copy link
Contributor

mergify bot commented Jan 18, 2025

This pull request, with head sha cbf960e5659054b2ccf27b67218782e69016bef5, has been successfully merged with fast-forward by Mergify.

This pull request will be automatically closed by GitHub.

As soon as GitHub detects that the sha cbf960e5659054b2ccf27b67218782e69016bef5 is part of the main branch, it will mark this pull request as merged.

It is possible for this pull request to remain open if this detection does not happen, this usually happens when a force-push is done on this branch flutter, this means GitHub will fail to detect the merge.

@mergify mergify bot merged commit cbf960e into nix-community:main Jan 18, 2025
4 checks passed
@GaetanLepage GaetanLepage deleted the flutter branch January 18, 2025 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MattSturgeon MattSturgeon MattSturgeon approved these changes

@khaneliman khaneliman khaneliman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[PLUGIN REQUEST] flutter-tools
4 participants
@GaetanLepage @khaneliman @traxys @MattSturgeon