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: OMB Improvements #270

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

nntoan
Copy link
Member

@nntoan nntoan commented Dec 24, 2021

  • bash-3.2 is the minimal version support. (macOS users no longer need to install another version of Bash like the first version of OMB)
  • Rename all .sh into .bash and add shebang line
  • Refactor to use OMB instead of OSH
  • Performance improvements for Git
  • .. to be continue

@nntoan nntoan added omb-next This issue has been fixed in the next oh_my_bash upgrade runs. improvement labels Dec 24, 2021
@nntoan nntoan changed the title OMB Improvements refactor: OMB Improvements Dec 24, 2021
Copy link
Contributor

@akinomyoga akinomyoga left a comment

Choose a reason for hiding this comment

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

Thank you for making the PR! In addition to the comments I directly made in the code,

  • I would like to notice that this major change will probably introduce conflicts to many existing PRs. We might need to notify the PR authors. Maybe we can resolve the conflicts at our side.
  • I think maybe we should change OMB to OMB_PATH, OMB_DIR, OMBDIR, or something more descriptive and unlikely to conflict with the variables defined by users and other people.
  • Just to confirm, is the shebang #!/usr/bin/env bash in plugins/themes/completions intended for text editors to correctly detect the language?
  • We want to import old configuration names as I have described in a reply at #268: e.g. : "${OMB_CUSTOM=${OSH_CUSTOM}}".
  • We want to keep oh-my-bash.sh as a symbolic link to oh-my-bash.bash for a while until the users complete the migration.

lib/base.bash Outdated Show resolved Hide resolved
lib/cli.bash Outdated Show resolved Hide resolved
lib/cli.bash Outdated Show resolved Hide resolved
lib/cli.bash Outdated Show resolved Hide resolved
lib/cli.bash Outdated Show resolved Hide resolved
lib/cli.bash Outdated Show resolved Hide resolved
themes/rjorgenson/rjorgenson.theme.bash Outdated Show resolved Hide resolved
themes/brainy/brainy.theme.bash Outdated Show resolved Hide resolved
tools/git-prompt.bash Outdated Show resolved Hide resolved
tools/install.bash Outdated Show resolved Hide resolved
@akinomyoga
Copy link
Contributor

If you are busy to address all of these comments, maybe I can work on them when I have time.

  • Refactor to use OMB instead of OSH
  • Performance improvements for Git

I feel we should separate the PRs for each point because I am likely to make many comments for each change.

This was referenced Dec 24, 2021
@akinomyoga
Copy link
Contributor

I found that the extension of the main script of oh-my-zsh is .sh despite that the extensions of the other scripts are .zsh. This is probably because oh-my-zsh already contains the shell name so oh-my-zsh.zsh would appear to be verbose. In this sense, oh-my-bash.bash also feels wordy/redundant.

@nntoan
Copy link
Member Author

nntoan commented Jan 6, 2022

I found that the extension of the main script of oh-my-zsh is .sh despite that the extensions of the other scripts are .zsh. This is probably because oh-my-zsh already contains the shell name so oh-my-zsh.zsh would appear to be verbose. In this sense, oh-my-bash.bash also feels wordy/redundant.

Agreed.

@akinomyoga
Copy link
Contributor

akinomyoga commented Jan 15, 2022

@nntoan Thanks. I think I need to share my current thinking for the plan to process this PR.

Currently, the author of PRs #288, #277, and #239 are working on polishing them up. On the other hand, this PR includes large changes *.{sh => bash} which will cause conflicts with these PRs. The change *.{sh => bash} in this PR is just renaming so is trivial to rework it. On the other hand, reworking these PRs #288, #277, and #239 requires effort. So I need to process the rename *.{sh => bash} after these PRs are merged.

Nevertheless, maybe the other changes in this PR can be processed separately (maybe in separate PRs). I'll take a look

@akinomyoga akinomyoga force-pushed the feature/omb-improvements branch 3 times, most recently from 8e9c179 to 2e90f1f Compare January 15, 2022 03:45
@akinomyoga
Copy link
Contributor

akinomyoga commented Jan 15, 2022

I've separated the commits.

Edit: I've rebased it on top of the latest master. The changes 4a608c7 (*.{sh => bash}) and 2e90f1f (OSH => OMB) are dropped tentatively. These changes will be reprocessed just before merging so as to avoid conflicts.

@akinomyoga
Copy link
Contributor

akinomyoga commented Jan 15, 2022

af20bfb I'm not sure if we should use #!/usr/bin/env bash as the shebang of all these script files that are supposed to be sourced from other script files. The point of shebang is to tell exec how to execute the file, but these script files are not intended to be executed as executable files, so they will fail if it is directly executed using Bash. Here's a related discussion:

What's the point of adding #!/usr/bin/env bash to those files that are intended to be sourced from interactive session of Bash? I searched what people do for this case and found an interesting solution using /bin/echo:

I suggest adding #!/bin/echo bash: usage: source , if we want to stick with shebang, or simply putting a normal comment # bash script to source. With the former suggestion, we can prevent the script from being directly executed so the script becomes safer. But actually, it shouldn't be a problem because the executable bit is unset in the file permission for these script files. Also, it is a hack and its intent is not clear at a glance. i actually prefer the latter.

Edit: I decided to tentatively use the shebang #! bash oh-my-bash.module. In this way, GNU Emacs sh-mode correctly detects the file type as a Bash script. Also, when someone mistakenly tried to directly execute the file, it produces an error expectedly, where the error message contains a string oh-my-bash.module so that the user knows it is a module file for oh-my-bash.

@akinomyoga akinomyoga force-pushed the feature/omb-improvements branch 3 times, most recently from c391560 to 2414c46 Compare January 15, 2022 10:03
akinomyoga
akinomyoga previously approved these changes Jan 15, 2022
@akinomyoga
Copy link
Contributor

@akinomyoga
Copy link
Contributor

akinomyoga commented Aug 26, 2024

Cross-Reference: This PR was mentioned in Discussion #591. (It is sad that the automatic cross-referencing doesn't work with GitHub Discussion)

@akinomyoga
Copy link
Contributor

I will pick the first three commits in the master.

Copy link

@obnoxxx obnoxxx left a comment

Choose a reason for hiding this comment

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

I think this PR does very many things at once.

Ideaaly, for my taste, a big feature like the introduction of a òmb`cli tool should not be hidden under a general ïmprovements subject but would deserve a dedicated PR title and commit message header.

Just my 2 cents as a user who is also a developer.

@akinomyoga
Copy link
Contributor

Yes, we already cropped commits from this PR several times...

I now picked three more commits from this PR and pushed them to the master branch. However, the implementation is actually incomplete. We now have a skeleton omb implementation in the master branch, but we need to implement each subcommand. Contributions are welcome.

To clarify the current status of this PR, although there is one remaining commit in the PR (766692e), there are also two commits 4a608c7 and 2e90f1f that are temporarily dropped to avoid unnecessary conflicts to any changes to the codebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement omb-next This issue has been fixed in the next oh_my_bash upgrade runs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants