Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

[Discussion] Enabling bash-it components to communicate messages to users #1501

Closed
davidpfarrell opened this issue Feb 21, 2020 · 13 comments · Fixed by #1623
Closed

[Discussion] Enabling bash-it components to communicate messages to users #1501

davidpfarrell opened this issue Feb 21, 2020 · 13 comments · Fixed by #1623

Comments

@davidpfarrell
Copy link
Contributor

[Adapted from recent Gitter posts]

Q: Seeing how bash-it appears to have a fail silently mantra to prevent unwanted terminal output, is there a means for bash-it to communicate issues/messages to the user?

At first I was thinking something simple like bash-it logs and give components a means for creating log messages to communicate error conditions (ie. functions like log_error, log_warning, etc)

But then I thought, in addition to that, maybe something like bash-it messages which would contain messages that components generated specifically for the user to see (where as logs could contain many types of messaging, auto-generated statuses, etc) ... (ie. user_message function)

THEN we could also design a prompt segment for letting the user know when messages are waiting to be read.

Also, feels like we could give ourselves permission to message the user when initiating a login session. Even if it was just to say something like "You have bash-it messages waiting to be read. Use 'bash-it messages' to read them."

@nwinkler
Copy link
Member

Good points! Here's the thing about failing silently. This was mostly based on the fact that error messages printed during Bash-it initialization were causing issues with things like scp, where the scp process would stall or fail due to the unexpected output.

If we can ensure that Bash-it is not initialized on non-interactive shell sessions (like scp or sftp), then we should be able to print some messages. I think some changes have been made (to bash_profile.template.bash to check for interactive shells - we would need to validate that this works as expected, and potentially do that check in bash_is.sh as well, so we don't rely on the user's profile.

If we can rule out that messages are printed in undesirable situations, we could add a couple of sensible messages. We certainly don't want to flood the user with dozens of messages, but a note that something is wrong might be OK.

Personally, I like the way things like Homebrew handle this. There's a brew doctor command that goes through some self checks and then shows what's wrong (and how it could be fixed). A bash-it doctor command could print the collected issues on demand.

This of course would mean that we introduce a logging system in Bash-it. When a plugin is enabled, but the required command is not installed, the plugin would need to log a message for that, which would be shown when you run bash-it doctor.

The bash-it doctor command would basically go through a full initialization of Bash-it, but instead of swallowing the log messages would print them, this could be triggered through an environment variable that is set when you run bash-it doctor.

So the following main blocks of work:

  • Provide an internal log function that by default doesn't print anything.
  • Provide a bash-it doctor command that loads the Bash-it config and prints log messages while going through the initalization.
  • Adjust all plugins to use the log function. As an alternative, we could embed the logging into the _command_exists function, which is supposed to be used in plugins anyway.

Thoughts?

@jpmcb
Copy link
Contributor

jpmcb commented Feb 24, 2020

+1 for bash-it doctor great idea. I think this would be super helpful, especially for plugin authors / contributors.

@rico-chet
Copy link
Contributor

rico-chet commented Mar 29, 2020

FYI the fix for non-interactive bash sessions, e.g. SCP, was this one: #1325

@cornfeedhobo
Copy link
Member

cornfeedhobo commented Mar 31, 2020

My $0.02 (only worth $0.01): I agree with @nwinkler. If we can ensure initialization is covered, it'd be great not to have silent failures. #1533 is a great first step (plus it fixes macos users' double initializing).

@nwinkler
Copy link
Member

@cornfeedhobo, @davidpfarrell, @rico-chet, @jpmcb - please see the implementation of the bash-it doctor command that @NoahGorny has done in #1623 - this is a great start.

Since there are no log statements so far, the command does not print anything at the moment.

As a start, I thought that we might want to add some output to the _command_exists function, since that should be used consistently for checking for dependencies, e.g. when initializing a plugin. I thought about adding a WARN message to that. Ideally, we find a way to also print the name of the respective plugin when calling the _command_exists function - either from the callstack, or through an additional (optional) parameter.

Other thoughts about places where logging makes sense?

@nwinkler nwinkler reopened this Jun 24, 2020
@davidpfarrell
Copy link
Contributor Author

davidpfarrell commented Jun 24, 2020

@nwinkler Thanks for the update - #1623 does look like a good start.

Some immediate thoughts:

Log Context

Ideally, we find a way to also print the name of the respective plugin

I'm thinking that the core bash-it code maintain a 'logging context' which will effectively act as a log message prefix at the time the message is logged.

I see it as an array of strings, ie ( 'plugins' 'git' ), can be pushed/popped by the core code as it transitions through its actions.
The logging function could then turn this into a prefix for the log message, ie plugins:git:DEBUG: message

Aborted Plugins

I think one of the first areas to add logging would be to plugins, adding error messages when plugins abort without activating (due to missing commands, folders, etc).

Logging in _command_exists

I think adding logging here is probably a good idea, especially in the short term where little/no other logging will exist.

I thought about adding a WARN message to that

Wondering if DEBUG might be more apropos, since the function itself doesn't know how the caller will treat a failure. It will also be noisy. I guess if we're concerned about it being the only logging in the short term, a case can be made for initially using WARN with a plan to shift to DEBUG over time.

That's all for now - Looking forward to seeing how this progresses !

-DF


cc: @NoahGorny

@NoahGorny NoahGorny mentioned this issue Jun 24, 2020
4 tasks
@nwinkler
Copy link
Member

@davidpfarrell Thanks for the summary - that looks good to me!

  • Log Context: Indeed, the Bash-it loader code could populate that, e.g. which plugin is being loaded at the moment.
  • _command_exists: DEBUG should be fine as well - we'll have to play with the noisiness (?) to tune it to the right amount. The current bash-it doctor implementation shows all messages (including DEBUG) by default.

@cornfeedhobo
Copy link
Member

If adding logging to _command_exists, we might want to include the caller.

@nwinkler
Copy link
Member

Since the logging has been added to _command_existsin #1628, this would be a new feature. FWIW, the logs include the concept of a BASH_IT_LOG_PREFIX that can be set to indicate who's creating the log entries at the moment. The bash_it.sh script uses that prefix to indicate e.g. which plugin is currently being loaded - that's almost like the caller.

A more advanced version with a callstack has been discussed above and in #1628, but was not implemented at the time. If someone wants to build a more advanced version, that would be cool.

@NoahGorny
Copy link
Member

This issue is very stale, so I close it

@gaelicWizard
Copy link
Contributor

I'd like to reopen this. An easy first step would be to just change the default from "fail silently" to "report errors and warnings" (set $BASH_IT_LOG_LEVEL=2), and cleanup any messages that aren't appropriate at their current levels. Silent errors are...not preferred... 😉

@NoahGorny
Copy link
Member

I am reopening- to leave this as a follow-up mission to enhance our logging information.
However- logging on every warning/error can be quite annoying IMO, maybe only on errors, and this is also debatable

@NoahGorny NoahGorny reopened this Aug 11, 2021
@gaelicWizard
Copy link
Contributor

logging on every warning/error can be quite annoying IMO, maybe only on errors, and this is also debatable

My mental model of logging levels (some of which Bash It doesn't have):
0. Fatal means we done forked up and better to just stop now; let the grownups clean up later.

  1. Errors mean something is broken and we should not continue the current action.
  2. Warnings mean something is unexpected in a way that might mean something is broken, but not necessarily, so handle it and keep going.
  3. Notice means ń̥o҉̙̻̖̲̮͓̩̀t̵̬̳͔ḩ̡̪͇͈i̼̦̮̙͝ng is wrong but the user should be told something.
  4. Info means normal things going on, only nerds would care.
  5. Debug is for developer tracing, like we're trying to find which line sets a variable.

I strongly feel that Bash It should run without errors unless something is broken. (There's still a lot of work to do before we can make this claim, but it's a reasonable near-term goal!)

If something is broken, Bash It should complain (loudly, IMO)!

Warnings I think should alsö be reported to the user, but again I alsö think that Bash It should run without warnings. This may be more of a medium-term goal.

Notice seems like what commands should use to tell the user things, like in plugins/proxy notifying that proxies are now enabled.


I'm working towards normal operation without errors or warnings, so hopefully that will come soon. 😃

@Bash-it Bash-it locked and limited conversation to collaborators Dec 5, 2022
@NoahGorny NoahGorny converted this issue into discussion #2181 Dec 5, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants