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

refactor all history modifications into discrete plugins #1764

Merged
merged 2 commits into from
Jan 16, 2021

Conversation

cornfeedhobo
Copy link
Member

Description

  • Moves lib/history.bash to history.plugin.bash
  • Removes the rh function since I couldn't figure out what real purpose it solves
  • Moves bash search options to their own plugins

Motivation and Context

The topic of searching substrings came up in #1506, and I was already looking at cleaning up lib/.

Fixes #1506

How Has This Been Tested?

Locally

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • If my change requires a change to the documentation, I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.
  • I have added tests to cover my changes, and all the new and existing tests pass.

@@ -0,0 +1,9 @@
cite about-plugin
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned in #1506, I think this should be one plugin + environment variables. Using two plugins will likely lead to further issues/questions down the road...

Copy link
Member Author

@cornfeedhobo cornfeedhobo Jan 5, 2021

Choose a reason for hiding this comment

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

  • We should aim for consistent naming patterns for plugin configuration options - don't think we have a consistent pattern at the moment.
  • How do we document plugin configuration settings? Do we want to support an optional documentation file per plugin?

I was thinking about that for a bit and came to the opposite conclusion. If I may make my case:

  • Discoverable: I don't think the average user should have to look through documentation to discover what's going on, or look at the plugin's source code. Since bash-it does not currently offer more than a single line description in a plugin, plugins with verbose names felt like a better trade off.

  • Clarity: I came to the opposite conclusion here. This PR would make enabling history modification more explicit, since there isn't a default modification baked in, and moves search modifications to their own plugins.

  • Load priority is taken care of because of lexographical ordering, giving substring search priority if both are enabled (tested locally).

I still would like to answer the question of plugin configuration and documentation, but I also think we should push back hard against them, because, IMHO, the goal should be to get out of the user's way as much and as fast as possible.

Also worth noting that I believe this broader topic is related to functionality in our themes as well; do we want environment variables as toggles? I think I've noticed that some of them are also basing logic on whether a plugin has been enabled or not?

Copy link

@mcarans mcarans Jan 5, 2021

Choose a reason for hiding this comment

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

I will leave it to you guys to figure out which is the best solution. My only input is that it should be as simple and clear for end users as possible. They should not need to look at source code and it would be great if each plugin had a clear description of its purpose and any configuration options in a file, on a wiki or whatever. Perhaps even a screenshot can be included as a picture often speaks more than words.

Those are my thoughts as an end user and thanks for your great work on this useful tool!

Copy link
Member

Choose a reason for hiding this comment

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

@cornfeedhobo Thanks for the detailed explanation of your thought process - makes sense to me. I'm fine with using your dual plugin approach. There's some code duplication in this case, but that shouldn't be much of an issue.

👍

@nwinkler
Copy link
Member

nwinkler commented Jan 6, 2021

One additional comment on this: Since this PR is moving the history settings from the implicitly loaded history.bash file in lib to a new plugin, we need to document this change in the release notes. People will want to enable the new history plugin so they have the same behavior as before.

Copy link
Member

@NoahGorny NoahGorny left a comment

Choose a reason for hiding this comment

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

The only changes needed are to bring rh back with some other name

export HISTSIZE=${HISTSIZE:-5000} # resize history size
export AUTOFEATURE=${AUTOFEATURE:-true autotest} # Cucumber / Autotest integration

function rh {
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs to return, like we discussed. Something like fav_commands?

Copy link
Member Author

Choose a reason for hiding this comment

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

haha don't worry, that's why i brought it up in the chat first 😄

Copy link
Member

Choose a reason for hiding this comment

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

I think we settled on a name now- if you change it, I will approve 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

@nwinkler @NoahGorny have a look

@cornfeedhobo
Copy link
Member Author

Okay. This branch has been rebased to master.

Before we proceed with merging, I want to point out the change to the output. It now is passed to column, which I think is available everywhere, but not sure if I should operate on that assumption. Do we want to check at the top of the plugin for awk and column before proceeding with the rest of the execution?

Copy link
Member

@NoahGorny NoahGorny left a comment

Choose a reason for hiding this comment

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

Well done, @cornfeedhobo!

@cornfeedhobo cornfeedhobo requested a review from nwinkler January 16, 2021 01:02
@NoahGorny NoahGorny merged commit 18781ed into Bash-it:master Jan 16, 2021
@cornfeedhobo
Copy link
Member Author

@NoahGorny I think this might need a follow-up PR to modify install.sh to enable history by default, since the functionality used to be in core. Also, the moving of the history search features to new plugin names will need some type of messaging upon upgrading.

@NoahGorny
Copy link
Member

@NoahGorny I think this might need a follow-up PR to modify install.sh to enable history by default, since the functionality used to be in core. Also, the moving of the history search features to new plugin names will need some type of messaging upon upgrading.

Hmm
Better open up an issue about it

@cornfeedhobo cornfeedhobo deleted the history-plugins branch February 3, 2021 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

History sub search in Bash
4 participants