-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
plugin: Add new onBufferOptionChanged
callback
#2962
base: master
Are you sure you want to change the base?
Conversation
runtime/plugins/linter/linter.lua
Outdated
@@ -66,7 +66,7 @@ function preinit() | |||
end | |||
|
|||
makeLinter("gcc", "c", "gcc", {"-fsyntax-only", "-Wall", "-Wextra", "%f"}, "%f:%l:%c:.+: %m") | |||
makeLinter("g++", "c++", "gcc", {"-fsyntax-only","-std=c++14", "-Wall", "-Wextra", "%f"}, "%f:%l:%c:.+: %m") | |||
makeLinter("g++", "c++", "g++", {"-fsyntax-only","-std=c++14", "-Wall", "-Wextra", "%f"}, "%f:%l:%c:.+: %m") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note: this change might be unnecessary (though will not hurt either), since it's basically the same compiler.
BTW... it might be a good idea to remove the -std=c++14
param, to support newer versions of the standard.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the forced standard is a good idea, to go with the actual GCC defined defaults.
Regarding the invoked checker/compiler I had different experiences, due to the different defaults applied by gcc vs g++. Exactly this was the reason why we receive unexpected linter messages when saving a C++ file. So I really recommend to go with the explicit GNU C++ compiler for C++ files.
feeda28
to
85bb039
Compare
internal/buffer/settings.go
Outdated
@@ -50,8 +52,16 @@ func (b *Buffer) DoSetOptionNative(option string, nativeValue interface{}) { | |||
return | |||
} | |||
|
|||
oldValue := b.Settings[option] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be useful for the above reflect.DeepEqual
check as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But, the old value was already checked.
Maybe I don't get, what you're trying to tell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant, we could move this line above the reflect.DeepEqual
check, and then we could use oldValue
in the reflect.DeepEqual
check as well, instead of doing lookup of b.Settings[option]
twice.
Moreover, I've just realized: since we've indeed already checked the equality above (and even in a more accurate way), it means that this oldValue != nativeValue
check is not needed at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of doing lookup of
b.Settings[option]
twice.
Ah yeah, got your point...since we realized in different discussions that this check is very expensive.
it means that this
oldValue != nativeValue
check is not needed at all?
Indeed, this wasn't present in the moment the PR was created.
85bb039
to
33244e4
Compare
This can become handy in the moment a plugin needs to react on e.g. changed file type.
33244e4
to
85e2db0
Compare
_, err := config.RunPluginFnBool(b.Settings, "onBufferOptionChanged", | ||
luar.New(ulua.L, b), | ||
luar.New(ulua.L, option), | ||
luar.New(ulua.L, string(oldValue.(string))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what happens when changing any non-string option:
Micro encountered an error: *runtime.TypeAssertionError interface conversion: interface {} is bool, not string
runtime/iface.go:261 (0x40a575)
github.com/zyedidia/micro/v2/internal/buffer/settings.go:127 (0x839adb)
github.com/zyedidia/micro/v2/internal/action/command.go:656 (0x87e9df)
github.com/zyedidia/micro/v2/internal/action/command.go:673 (0x87ec59)
github.com/zyedidia/micro/v2/internal/action/command.go:703 (0x87ef94)
github.com/zyedidia/micro/v2/internal/action/command.go:1140 (0x882308)
github.com/zyedidia/micro/v2/internal/action/actions.go:1859 (0x870d72)
github.com/zyedidia/micro/v2/internal/info/infobuffer.go:163 (0x840463)
github.com/zyedidia/micro/v2/internal/action/infopane.go:223 (0x886d2c)
github.com/zyedidia/micro/v2/internal/action/infopane.go:54 (0x885c36)
github.com/zyedidia/micro/v2/internal/action/infopane.go:132 (0x8865a2)
github.com/zyedidia/micro/v2/internal/action/infopane.go:91 (0x88616c)
github.com/zyedidia/micro/v2/cmd/micro/micro.go:482 (0x8c4425)
github.com/zyedidia/micro/v2/cmd/micro/micro.go:400 (0x8c3e3f)
runtime/proc.go:255 (0x436ca7)
runtime/asm_amd64.s:1581 (0x465de1)
If you can reproduce this error, please report it at https://github.com/zyedidia/micro/issues
Why are you even converting it to string?
With the introduction of the new callback it's possible for plugins to react upon changed buffer settings.
Especially the
linter
plugin will benefit by this change, since it can the properly unregister messages created with a different linter type.Fixes #2961
Fixes #3587