Skip to content

Conversation

@saygo-png
Copy link
Contributor

This is an automated change using a linter I wrote. I glanced at every change and git added it one by one but I might have missed some things.

It checks for cases:

foo = "nil"; # -> foo.__raw = "nil";
foo = null;  # -> foo.__raw = "nil";
foo = [];    # -> foo.__empty = {};
foo = {};    # -> foo.__empty = {};

This check is performed in any attribute set bound to a key "settings". So there were a couple of false positives in options where settings was not the kind that gets converted to Lua.

If I missed some case i can write a rule for it and reapply.

@nixvim-ci nixvim-ci bot requested a review from GaetanLepage October 27, 2025 17:48
@saygo-png
Copy link
Contributor Author

saygo-png commented Oct 27, 2025

A lot of failures. Looks like null was used to mean nil, "empty table" and "ignore this" which is now causing problems. I'm considering skipping the null check.

There also seems to be a lot of settings options that don't expect raw Lua. Is this intended? Shouldn't we allow any value to be replaced with raw Lua?

@saygo-png saygo-png marked this pull request as draft October 31, 2025 17:05
@saygo-png saygo-png force-pushed the use-raw branch 5 times, most recently from 373398c to 5a4effd Compare October 31, 2025 22:51
@MattSturgeon
Copy link
Member

There also seems to be a lot of settings options that don't expect raw Lua. Is this intended? Shouldn't we allow any value to be replaced with raw Lua?

This is a known issue. Essentially, rawLua support in custom/bespoke options needs to be added manually. It's quite easy to miss some permutations, especially in composite types where you may have (e.g.) "null, or raw lua, or list of (raw lua or something)".

So yes, there are a bunch of options/types that should allow raw lua but don't. Updating the tests is a good way to catch many of these, but I don't feel like it should be this PR's responsibility to fix the issues it unearths.

@saygo-png
Copy link
Contributor Author

I don't feel like it should be this PR's responsibility to fix the issues it unearths.

What should I do then?
My current plan is to cherrypick the fixes into separate PRs and rebase this one until it passes. Somewhat grouped PRs since a lot of these changes are going to be small.

@MattSturgeon
Copy link
Member

The minimum would be to flag all problematic options with FIXME and otherwise omit them from this PR. If you don't want to do that, then we will have to fix or drop all problematic options.

@saygo-png
Copy link
Contributor Author

The minimum would be to flag all problematic options with FIXME and otherwise omit them from this PR. If you don't want to do that, then we will have to fix or drop all problematic options.

Ok. I'll mark the difficult to solve ones with FIXME, but the trivial ones i'll just fix/drop and cherrypick. A difficult one is for example none-ls with the settings.sources option as it's used for the autoinstall implementation and is expected to be a list.

I'm not sure what to do about options that coerce strings to raw Lua. I can't provide a raw value for those because it tries to coerce it. I can't fix it because there is no way to deprecate these without breaking configs. I can just write foo = "nil" and that will get coerced but is this something that should be done? Do i just leave it out and mark it with FIXME? Marking it feels a bit redundant because it's a limitation of every coercing option but I also don't want to leave it to discourage the usage of null in tests.

@saygo-png saygo-png force-pushed the use-raw branch 3 times, most recently from f2223ef to 56ce248 Compare November 2, 2025 00:11
@MattSturgeon
Copy link
Member

MattSturgeon commented Nov 2, 2025

I'm not sure what to do about options that coerce strings to raw Lua.

We're well overdue deprecating that behaviour properly, in the various types and option-factories that implement the coercion.

We can probably just do it by adding lib.warn (or later throw) when applying the coercion, since I'm guessing it'll be difficult to define the warnings or assertions option conditionally based on whether there is deprecated coercion going on.

Another thing I've been meaning to do for a long time, but haven't gotten around to yet.

Do i just leave it out and mark it with FIXME?

That is acceptable for the purposes of this PR. Deprecating this coercion is a massive can-of-worms on its own!

@saygo-png saygo-png force-pushed the use-raw branch 3 times, most recently from 5569a50 to b6aea3e Compare November 3, 2025 01:43
@saygo-png
Copy link
Contributor Author

saygo-png commented Nov 3, 2025

sooo many failures D:
I fix some and then it just uncovers more as the build goes further....

@saygo-png
Copy link
Contributor Author

Finally green! Let me just clean up these commits...

Signed-off-by: saygo-png <saygo.mail@proton.me>
Signed-off-by: saygo-png <saygo.mail@proton.me>
Signed-off-by: saygo-png <saygo.mail@proton.me>
Signed-off-by: saygo-png <saygo.mail@proton.me>
Signed-off-by: saygo-png <saygo.mail@proton.me>
Signed-off-by: saygo-png <saygo.mail@proton.me>
Signed-off-by: saygo-png <saygo.mail@proton.me>
Signed-off-by: saygo-png <saygo.mail@proton.me>
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.

2 participants