fix: #657 don't show error message for optional integrations #658
Conversation
| end | ||
| end, | ||
| ["d"] = function() | ||
| if not config.ensure_integration("diffview") then |
There was a problem hiding this comment.
instead of a boolean param, can we do an opts table, like { required = x }? Not a huge fan of naked boolean params. Thats how you wind up with a function signature like call("string", true, false, true true) :P
There was a problem hiding this comment.
Yes. Not entirely sure about the existing name for the function either.
ensure_, at least the way I've encountered and used the name refers to a function which does a check and in some way ensures a value, such as generating one if it doesn't exist, ensures a column is present in a database etc.
This is essentially a query or check function, especially when adding a { required = true }
Perhaps query_integration(name) -> bool and the have the caller do an error or fallback, with better context, such as diffview integration needs to be enabled for "diff head"
There was a problem hiding this comment.
I like the idea of leaving it up to the caller. That removes the need for the { required = true } param altogether, and... wait for it... we could call it... check_integration(name). Because it's a check.. for the integration. :P
There was a problem hiding this comment.
Great. Often great solutions come from stepping back and looking at the larger picture, rather than adding yet another bool argument
There was a problem hiding this comment.
Made the change to check and provided some more descriptive error messages that show a notification.
Just fixed a call("string", true, false, true true) situation where a big async function begun with an if statement, that either allocated a scratch buffer and did a thing, or did a thing directly. Just pried it apart into two functions, as the bools were never dynamic but always hardcoded anyway
There was a problem hiding this comment.
Yeahhh. Thats something I remember from.. I think Uncle Bob... early on in my career. If you are controlling a functions behaviour externally with boolean flags... congrats, you have two functions :P
| return ok and value or false | ||
| end, | ||
| }), | ||
| integrations = { |
There was a problem hiding this comment.
The previous auto integration relies on the metatable being left intact and would break if the table was replaced. Additionally, this makes the config data only and does not muddle printing or probing of the config with runtime determined fields or obscuring what the user originally said with what we found through pcall.
This functionality is now ensured by the check_integration function
|
What do you think of this @CKolkey |
CKolkey
left a comment
CKolkey
left a comment
There was a problem hiding this comment.
Minor change, then good to go :)
lua/neogit/config.lua
Outdated
| end | ||
|
|
||
| return true | ||
| logger.fmt_info("Found explicit integration '%s' = %s", name, enabled) |
There was a problem hiding this comment.
I'd like to start prefixing these with [LOCATION] to make reading the log easier. So, [CONFIG] in this case
No description provided.