plugins/todo-comments: migrate to mkNeovimPlugin by khaneliman · Pull Request #2030 · nix-community/nixvim

khaneliman · 2024-08-17T03:50:32Z

No description provided.

plugins/utils/todo-comments.nix

MattSturgeon · 2024-08-17T18:28:36Z

plugins/utils/todo-comments.nix

-        (mkIf (cfg.keymaps.todoTrouble != null) { trouble.enable = true; })
-        (mkIf (cfg.keymaps.todoTelescope != null) { telescope.enable = true; })
-      ];
+          mkCompositeOption "Keymap settings for the `:${funcName}` function." {


Is it possible to do this is a way that complies with #603?

If you're unfamiliar, the idea is that plugin keymaps should make use of mkMapOptionSubmodule instead of implementing their own keymap submodule types.

If mkMapOptionSubmodule is too limiting, because it doesn't support additional options (cwd, keywords) I'm open to making it more powerful by adding an extraOptions argument or similar.

As discussed in #2030 (comment) this can done in be another PR

plugins/utils/todo-comments.nix

MattSturgeon · 2024-08-17T21:07:57Z

plugins/utils/todo-comments.nix

+    keymaps = flatten (
+      mapAttrsToList (
+        optionName: funcName:
+        let
+          keymap = cfg.keymaps.${optionName};
+
+          cwd = optionalString (keymap.cwd != null) " cwd=${keymap.cwd}";
+          keywords = optionalString (keymap.keywords != null) " keywords=${keymap.keywords}";
+        in
+        optional (keymap != null) {
+          mode = "n";
+          inherit (keymap) key options;
+          action = ":${funcName}${cwd}${keywords}<CR>";
+        }
+      ) commands
+    );


I see this is unchanged from the previous implementation, other than being indented further.

However, I think this would be simpler mapping over cfg.keymaps instead of mapping over commands?

Ideally, I'd like to avoid using optional+flatten, perhaps we could do something like:

keymaps = lib.pipe cfg.keymaps [ (filterAttrs (n: v: v != null)) (mapAttrsToList (name: keymap: { inherit (keymap) key option; mode = "n"; action = let cmd = commands.${name}; cwd = optionalString (keymap.cwd != null) " cwd=${keymap.cwd}"; keywords = optionalString (keymap.keywords != null) " keywords=${keymap.keywords}"; in "<cmd>${cmd}${cwd}${keywords}<cr>"; })) ];

Looking at the commands attrset, we could probably replace it with a upperFirstChar style helper.

However, many of the details would be slightly different if we can go down the #603 route.

I see this is unchanged from the previous implementation, other than being indented further.

Yeah, sorry I didn't touch this bit. I wasn't sure what to do with it, yet.

I think the suggested solution is a lot cleaner. But, I need to get more familiar with that keymap issue that was mentioned here and above.

We can always tackle it in another PR, but the other suggestions above could be better.

If I can tackle that separately it would give me a bit to figure it out and maybe get the extraOptions support for mkMapOptionSubmodule

plugins/utils/todo-comments.nix

MattSturgeon

Think these are the final comments 🚀

MattSturgeon · 2024-08-18T15:11:47Z

plugins/utils/todo-comments.nix

+  keymapsActions = {
    todoQuickFix = "TodoQuickFix";
    todoLocList = "TodoLocList";
    todoTrouble = "TodoTrouble";
    todoTelescope = "TodoTelescope";
  };


Up to you, we could drop this, since we have lib.nixvim.upperFirstChar:

nixvim/lib/utils.nix

Lines 81 to 104 in 78fc4be

/**

Capitalize a string by making the first character uppercase.

# Example

```nix

upperFirstChar "hello, world!"

=> "Hello, world!"

```

# Type

```

upperFirstChar :: String -> String

```

*/

upperFirstChar =

s:

let

first = substring 0 1 s;

rest = substring 1 (stringLength s) s;

result = (toUpper first) + rest;

in

optionalString (s != "") result;

Well, I guess it's used in the option declaration as well as the keymap transformation. So maybe best to keep it?

MattSturgeon · 2024-08-18T15:15:04Z

plugins/utils/todo-comments.nix

+          mode = "n";
+          action =
+            let
+              cmd = keymapsActions.${name};


If you decide to drop the keymapsActions attrs:

Suggested change

cmd = keymapsActions.${name};

cmd = lib.nixvim.upperFirstChar name;

plugins/utils/todo-comments.nix

MattSturgeon · 2024-08-18T15:36:47Z

plugins/utils/todo-comments.nix

+    keywords =
+      defaultNullOpts.mkAttrsOf
+        (types.submodule {
+          options = {


Note: this will be affected by #1618, however that doesn't affect how you should implement it here. It's just another reason for us to investigate the issue.

plugins/utils/todo-comments.nix

MattSturgeon

Can't see any remaining issues. Thanks for another quality PR! 🎉

Using mkMapOptionSubmodule as per #603 can be postponed to another PR, and may be best done after #2035 is merged anyway.

Up to you if you're happy merging now if if you'd like to ping other maintainers for a second opinion.

No need to wait for the tests to pass though, since mergify will do that for us.

khaneliman · 2024-08-18T19:25:18Z

@Mergifyio queue

mergify · 2024-08-18T19:25:43Z

queue

🛑 The pull request has been removed from the queue `default`

Details

The pull request #2030 has been manually updated.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

MattSturgeon · 2024-08-18T19:32:59Z

@Mergifyio refresh

MattSturgeon · 2024-08-18T19:33:05Z

@Mergifyio queue

mergify · 2024-08-18T19:33:34Z

refresh

✅ Pull request refreshed

mergify · 2024-08-18T19:33:35Z

queue

✅ The pull request has been merged automatically

Details

The pull request has been merged automatically at 379ae77

khaneliman force-pushed the todo branch 5 times, most recently from 8ecf733 to 3fb4211 Compare August 17, 2024 05:31

khaneliman marked this pull request as ready for review August 17, 2024 05:33

khaneliman mentioned this pull request Aug 17, 2024

Fix for obsolete nerdfont icons #2016

Closed

MattSturgeon reviewed Aug 17, 2024

View reviewed changes

plugins/utils/todo-comments.nix Outdated Show resolved Hide resolved

MattSturgeon reviewed Aug 17, 2024

View reviewed changes

plugins/utils/todo-comments.nix Show resolved Hide resolved

MattSturgeon reviewed Aug 17, 2024

View reviewed changes

plugins/utils/todo-comments.nix Outdated Show resolved Hide resolved

MattSturgeon reviewed Aug 17, 2024

View reviewed changes

plugins/utils/todo-comments.nix Show resolved Hide resolved

MattSturgeon reviewed Aug 17, 2024

View reviewed changes

khaneliman force-pushed the todo branch 2 times, most recently from 826f39a to e550f80 Compare August 17, 2024 20:18

MattSturgeon reviewed Aug 17, 2024

View reviewed changes

khaneliman force-pushed the todo branch 3 times, most recently from 970e1df to f602326 Compare August 18, 2024 05:11

MattSturgeon requested changes Aug 18, 2024

View reviewed changes

khaneliman force-pushed the todo branch from f602326 to a41eb2e Compare August 18, 2024 17:30

MattSturgeon reviewed Aug 18, 2024

View reviewed changes

plugins/utils/todo-comments.nix Outdated Show resolved Hide resolved

MattSturgeon reviewed Aug 18, 2024

View reviewed changes

plugins/utils/todo-comments.nix Outdated Show resolved Hide resolved

MattSturgeon reviewed Aug 18, 2024

View reviewed changes

plugins/utils/todo-comments.nix Outdated Show resolved Hide resolved

khaneliman force-pushed the todo branch 4 times, most recently from eb58f12 to a8437a8 Compare August 18, 2024 18:17

khaneliman requested a review from MattSturgeon August 18, 2024 18:23

MattSturgeon approved these changes Aug 18, 2024

View reviewed changes

plugins/todo-comments: migrate to mkNeovimPlugin

379ae77

khaneliman force-pushed the todo branch from a8437a8 to 379ae77 Compare August 18, 2024 19:25

mergify bot merged commit 379ae77 into nix-community:main Aug 18, 2024

mergify bot temporarily deployed to github-pages August 18, 2024 19:37 Inactive

khaneliman deleted the todo branch August 19, 2024 02:58

khaneliman mentioned this pull request Aug 20, 2024

plugins/todo-comments: migrate keymaps to mkMapOptionSubmodule #2046

Merged

	/**
	Capitalize a string by making the first character uppercase.

	# Example

	```nix
	upperFirstChar "hello, world!"
	=> "Hello, world!"
	```

	# Type

	```
	upperFirstChar :: String -> String
	```
	*/
	upperFirstChar =
	s:
	let
	first = substring 0 1 s;
	rest = substring 1 (stringLength s) s;
	result = (toUpper first) + rest;
	in
	optionalString (s != "") result;

	cmd = keymapsActions.${name};
	cmd = lib.nixvim.upperFirstChar name;

Uh oh!

Conversation

khaneliman commented Aug 17, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MattSturgeon Aug 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MattSturgeon Aug 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MattSturgeon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MattSturgeon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khaneliman commented Aug 18, 2024

Uh oh!

mergify bot commented Aug 18, 2024

🛑 The pull request has been removed from the queue default

Uh oh!

MattSturgeon commented Aug 18, 2024

Uh oh!

MattSturgeon commented Aug 18, 2024

Uh oh!

mergify bot commented Aug 18, 2024

✅ Pull request refreshed

Uh oh!

mergify bot commented Aug 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ The pull request has been merged automatically

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

MattSturgeon Aug 17, 2024 •

edited

Loading

MattSturgeon Aug 17, 2024 •

edited

Loading

MattSturgeon left a comment •

edited

Loading

🛑 The pull request has been removed from the queue `default`

mergify bot commented Aug 18, 2024 •

edited

Loading