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.

Sign up for GitHub

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

Jump to bottom

Wholesale cleanup of nodejs related components #1990

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Wholesale cleanup of nodejs related components #1990

wants to merge 2 commits into from

Conversation

cornfeedhobo
Copy link
Member

Description

  • remove redundant cite
  • support Basher
  • add nenv plugin
  • uplift nodenv to follow newer pattern and standard
  • ensure node components play well with *env tools

Motivation and Context

I had to work on node and ran into these issues.

How Has This Been Tested?

locally

Screenshots (if appropriate):

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.

@cornfeedhobo
Copy link
Member Author

cornfeedhobo commented Dec 2, 2021 •
edited
Loading

Does anyone have a strong feeling about leaving https:// in the about-plugin blurb?

Unfortunately the failing test is indicating an unrelated bug that should probably be addressed first.

Edit: I'm unable to reproduce the bug locally but I know I've seen bad search on ubuntu hosts before

cc @NoahGorny @davidpfarrell @gaelicWizard

@gaelicWizard
Copy link
Contributor

gaelicWizard commented Dec 27, 2021 •
edited
Loading

In my helpers PR (#1934), I have some fixes related to bash-it search (via _bash-it-describe and boy is that code squirrelly); not sure if that's relevant to this test failure.

I don't have feelings about https:// (for once!)

@cornfeedhobo
Wholesale cleanup of nodejs related components
bce2b41 
- remove redundant cite
- support Basher
- add nenv plugin
- uplift nodenv to follow newer pattern and standard
- ensure node components play well with *env tools
@cornfeedhobo
Copy link
Member Author

@gaelicWizard Unfortunately it appears that the search code is still broken :(

@gaelicWizard
Copy link
Contributor

gaelicWizard commented Feb 23, 2022 •
edited
Loading

@cornfeedhobo, "management" contains the substring "gem":

$ bash-it show plugin |grep gem
nenv                 [ ]        Node.js environment management using https://github.com/ryuone/nenv
nodenv               [ ]        Node.js environment management using https://github.com/nodenv/nodenv
ruby                 [ ]        ruby and rubygems specific functions and settings

So, bash-it search gem returns nenv and nodenv.

@cornfeedhobo
Wholesale cleanup of nodejs related components
88a5ea5 
- remove redundant cite
- support Basher
- add nenv plugin
- uplift nodenv to follow newer pattern and standard
- ensure node components play well with *env tools
@cornfeedhobo
Copy link
Member Author

@gaelicWizard Hey I finally have a free weekend to think. Sorry I fired off that comment without taking a real look.

Yeah, I see today that search is checking descriptions, which is unexpected, but wfm. Thanks for pointing it out and saving me the hunting.

NoahGorny
NoahGorny reviewed Mar 8, 2022
View reviewed changes
plugins/available/nvm.plugin.bash
if ! _command_exists nvm
then
function nvm() {
echo "Bash-it no longer bundles the nvm script. Please install the latest version from"
Copy link
Member

Choose a reason for hiding this comment

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

what about logging a warning in the case we dont find nvm.sh?

plugins/available/node.plugin.bash
about-plugin 'Node.js helper functions'

# Check that we have npm
_command_exists npm || return
Copy link
Member

Choose a reason for hiding this comment

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

shouldnt we still check this?

@seefood
Copy link
Contributor

seefood commented Nov 7, 2024

Want to fix the merge conflicts and unresolved remarks from Noah?/

@seefood seefood self-assigned this Nov 7, 2024
@seefood seefood added clean up seems abandoned rattle the cage, and close if nobody wants to keep it going waiting-for-response labels Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NoahGorny NoahGorny NoahGorny left review comments

@gaelicWizard gaelicWizard Awaiting requested review from gaelicWizard

@davidpfarrell davidpfarrell Awaiting requested review from davidpfarrell

At least 1 approving review is required to merge this pull request.

Assignees

@seefood seefood

Labels
clean up seems abandoned rattle the cage, and close if nobody wants to keep it going waiting-for-response
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cornfeedhobo @gaelicWizard @seefood @NoahGorny