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

Add load/reloader logs #1628

Merged
merged 10 commits into from
Jul 7, 2020
Merged

Add load/reloader logs #1628

merged 10 commits into from
Jul 7, 2020

Conversation

NoahGorny
Copy link
Member

@NoahGorny NoahGorny commented Jun 24, 2020

Continuation of #1501
Also add prefix option for the logs, not by a stack, but rather by a variable set by the owning file

TODO:

  • Add logs to plugins
  • Unset prefix on closure of each file
  • Add logs to completions?
  • Add logs to themes?

Also add a new test to check it
@davidpfarrell
Copy link
Contributor

davidpfarrell commented Jun 25, 2020

Nice work knocking out a version of this already !

I suppose a simple string prefix is usable as my initial stack idea.

However, I don't think I would expect each individual alias/plugin/completion/theme/etc file set the prefix directly. I think the bash-it.sh script can set the prefix right before sourcing the individual files:

# libraries, but skip appearance (themes) for now
_log_debug "Loading libraries(except appearance)..."
LIB="${BASH_IT}/lib/*.bash"
APPEARANCE_LIB="${BASH_IT}/lib/appearance.bash"
for _bash_it_config_file in $LIB
do
  if [ "$_bash_it_config_file" != "$APPEARANCE_LIB" ]; then

    BASH_IT_LOG_PREFIX="library: ${_bash_it_config_file}"  # TODO - strip path prefix and .bash suffix

    # shellcheck disable=SC1090
    source "$_bash_it_config_file"
  fi
done

Here, each library gets the context (prefix) for free and only has to worry about logging messages.

Just a few of these sprinkled throughout the bash-it core code would go a long way.

@nwinkler
Copy link
Member

Yes, I agree with @davidpfarrell - setting the prefix variable in the loader code loops (in reloader.bash) would take care of a good chunk of the work. Together with the log message in _command_exists, it should provide good initial coverage.

For the reloader code, it would imagine it working like this:

  • Print a debug message about loading stuff from the enabled directory.
  • For each found file/link:
    ** Print a debug message about loading that file
    ** Set the prefix variable to something like plugin: $plugin_name - getting the plugin_name from the file name should probably be provided in a function - don't think we have that yet. The bash-it.completion.bash file uses this: basename $f | sed -e 's/\(.*\)\..*\.bash/\1/g' | sed -e "s/^[0-9]*---//g"
    ** Load the file
  • After the loop reset the prefix variable to its previous value

Thoughts?

@davidpfarrell
Copy link
Contributor

davidpfarrell commented Jun 25, 2020

The bash-it.completion.bash file uses this: basename $f | sed -e 's/\(.*\)\..*\.bash/\1/g' | sed -e "s/^[0-9]*---//g"

What are we, animals !?

# full component filename with path, priority and extension
$ filename="/home/user/.bashit/enabled/123---awesome-component.type.bash"

# filename without path
$ filename=${filename##*/}

123---awesome-component.type.bash

# filename without path or priority
$ filename=${filename##*---}

awesome-component.type.bash

# filename without path, priority or extension
$ filename=${filename%.*.bash}

awesome-component

After the loop reset the prefix variable to its previous value

This is kinda what my stack idea as meant to address.

I wouldn't want the code at the loop location to hard-code the previous value. We might be able to use a backup variable to store/restore the value, but you'll want the backup variable name to be unique to the loop so that if the call path enters a second loop somewhere that also needs to store/restore the context, it doesn't trash your first backup value.

You could also use this backup variable to abstract global context from the local loops:

# Store global context so we can restore it later
BASH_IT_LOG_PREFIX_BAK_LIB=${BASH_IT_LOG_PREFIX}

for do
    # Add our local context, maintaining global context
    BASH_IT_LOG_PREFIX="${BASH_IT_LOG_PREFIX_BAK_LIB}:library: ${_bash_it_config_file}"
done

# Restore global context before moving on
BASH_IT_LOG_PREFIX=${BASH_IT_LOG_PREFIX_BAK_LIB}

Again, my stack idea was meant to enable these concepts using simpler push/pop semantics.

Overall I think we're heading in a good direction !

-DF

@NoahGorny NoahGorny force-pushed the add-load-logs branch 2 times, most recently from 23f7a7b to 8e34916 Compare June 28, 2020 08:30
@NoahGorny
Copy link
Member Author

I did not yet implemented stack or even prev backup, but now there are many log messages and the prefix are set up correctly
I might add some tests to see that it works as expected

Copy link
Member

@nwinkler nwinkler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great - I've left some comments, please take a look.

@NoahGorny NoahGorny marked this pull request as ready for review June 30, 2020 17:58
Copy link
Member

@nwinkler nwinkler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the back and forth, didn't mean it to be this complicated, but I think it's important that we get this right... Some more review comments.

@nwinkler
Copy link
Member

nwinkler commented Jul 3, 2020

Thanks for working through this - looks great now! Is this ready-to-merge from your perspective?

@davidpfarrell Comments, thoughts?

@NoahGorny
Copy link
Member Author

Thanks so much for reviewing!
I think this is a nice start, and we should start to recommend adding logs to anyone who opens a PR. I think we can merge this, but let's wait for more people to chime in 😄

In any case, we can add more logs to various components whenever we want to

@nwinkler nwinkler merged commit 2814946 into Bash-it:master Jul 7, 2020
@nwinkler
Copy link
Member

nwinkler commented Jul 7, 2020

Merging this now - we can keep adding more logs as needed.

@nwinkler
Copy link
Member

nwinkler commented Jul 7, 2020

Thanks again for the implementation, @NoahGorny - this is great!

@NoahGorny
Copy link
Member Author

Thanks for merging @nwinkler
Happy to help 😊

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.

3 participants