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

command: Prevent re-writing settings in case of local option #3178

Merged
merged 2 commits into from
Apr 11, 2024

Conversation

JoeKar
Copy link
Collaborator

@JoeKar JoeKar commented Mar 15, 2024

This is a follow-up of #3042.

@dmaluka
Copy link
Collaborator

dmaluka commented Mar 15, 2024

LGTM. I'd also suggest a small refactoring of SetGlobalOptionNative() as a follow-up: deal with local at the beginning and be done with it, before dealing with global:

if local {
	MainTab().CurPane().Buf.SetOptionNative(option, nativeValue)
	return nil
}

@JoeKar JoeKar force-pushed the fix/unnecessary-write-settings branch from e71dd1f to 020bdd0 Compare March 17, 2024 16:11
internal/action/command.go Outdated Show resolved Hide resolved
@JoeKar JoeKar force-pushed the fix/unnecessary-write-settings branch from 020bdd0 to 5fff4d3 Compare March 22, 2024 18:05
@JoeKar JoeKar requested a review from dmaluka April 8, 2024 18:36
@dmaluka
Copy link
Collaborator

dmaluka commented Apr 10, 2024

Upon a fresh look, it's also kinda weird that we check (and "properly handle") a local option inside a function named SetGlobalOptionNative(), as a special case, rather than checking that in the caller function. Or are we checking that both here and in the caller function? (I'm afraid that's what we actually do, and also I'm afraid that we cannot simply remove this redundancy, as it would break something.)

But that's just a lyrical note.

@JoeKar
Copy link
Collaborator Author

JoeKar commented Apr 10, 2024

But that's just a lyrical note.

What about renaming SetGlobalOption() into SetOption(), but still exporting it as SetGlobalOption() via Lua for the plugins?
Then the call can be moved to the calling function and would fit there more. The native value can be obtained with the help of config.DefaultCommonSettings() as it's done in ResetCmd().
The only problem and most probably breaking compatibility then is the fact that the exported plugin function SetGlobalOptionNative() doesn't check for locals any longer.

So the problem was to export it as "Global" with the reference as with > set, while > set can be used to change locals too. :(

Sh**, sometimes it could be easier...

Edit:
This is anyway something which could be done separately, if we really plan to do something in that direction.
So far we prevent rewriting the settings.

@JoeKar JoeKar merged commit 426aa9b into zyedidia:master Apr 11, 2024
3 checks passed
@JoeKar JoeKar deleted the fix/unnecessary-write-settings branch April 11, 2024 16:35
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