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

aliases: Move alias plugins to aliases #602

Closed
wants to merge 14 commits into from

Conversation

RobLoach
Copy link
Contributor

@RobLoach RobLoach commented Sep 5, 2024

This moves all the remaining plugins that just implement aliases to the aliases directory.


Note by @akinomyoga: This is superseded by the new PR #609.

@akinomyoga
Copy link
Contributor

Thank you for these suggestions!

There is a problem that is difficult for me to decide. Although I'd like to move these plugins to aliases as suggested, there is a compatibility issue. The existing users who have already enabled these plugins will see errors. In addition, for the users who've forked a branch and modified the plugins, it would be more non-trivial to merge the changes. Technically, we can inform such users to fix their .bashrc and merge conflicts to make it consistent with our changes. However, I'm not sure if it would be worth moving these plugins to aliases at the expense of breaking existing users' settings and requiring the users to fix the settings.

The reason that I suggested moving "plugin cargo" to "aliases cargo" was because it is a new plugin just added a few weeks ago and disabled by default. Therefore, we expect there are not so many existing users of the plugin cargo.

A possible workaround would be to leave symbolic links to the new locations in the plugin directories. However, I'm wondering if such a complication is needed at all.

Honestly, there are many inconsistencies in this project, and the present inconsistency of plugins/aliases is merely one of them. To solve them, all kinds of destructive changes in Oh My Bash are needed. Continuously breaking compatibilities doesn't seem nice, but it requires a large amount of work to solve everything all at once. Even if it is done all at once, we would anticipate the questions and reports from the users who are troubled by the incompatible changes. I currently don't have the bandwidth to handle these issues. What do you think about the compatibility?

@akinomyoga
Copy link
Contributor

Separate PRs are created for each plugin, but the same type of changes under the same motivation and reasoning are better done in a single PR and merged into the main branch at once.

@akinomyoga akinomyoga added the omb-next This issue has been fixed in the next oh_my_bash upgrade runs. label Sep 5, 2024
@RobLoach
Copy link
Contributor Author

RobLoach commented Sep 5, 2024

I have no strong opinion either way. Backwards compatibility is a good reason to keep them where they are. In any case, I've gone through the remaining plugins that just implement aliases and moved them to an alias file.

@RobLoach RobLoach changed the title aliases/kubectl: Move kubectl to aliases aliases: Move alias plugins to aliases Sep 5, 2024
aliases/brew.aliases.md Outdated Show resolved Hide resolved
@akinomyoga
Copy link
Contributor

akinomyoga commented Sep 5, 2024

Thank you for updating, but I forgot to mention another point to consider. I'm sorry, I should have commented on that earlier.

Since this project is a Bash version of Oh My Zsh (OMZ) and is actually derived from OMZ, we want to keep some consistency with OMZ even if we do not offer perfect parity with OMZ. This is because the OMB users are often also users of OMZ and use OMB to set up a similar environment as their environment in Zsh with OMZ. The upstream OMZ doesn't have a category "aliases", and everything including aliases is inside "plugins". The golang and kubectl plugins that this PR tries to move have corresponding files in the upstream OMZ. We wouldn't like to break this correspondence.

OMZ also seems to have the brew plugin, but the upstream brew seems different from ours, so it's actually better to move our brew to aliases not to confuse the users of both OMB/OMZ:

Another upstream of this project is Bash-it. The category aliases originates in Bash-it. Bash-it seems to have kubectl.aliases.bash, but its contents appear to show that it has a different origin.

@RobLoach
Copy link
Contributor Author

RobLoach commented Sep 6, 2024

Didn't know parity was desired, thanks for the explanation. Does this sound like a good path forwards?

  • Move our Chezmoi to aliases, since neither ohmyzsh or bash-it implement it as a plugin.
  • Move our brew plugin to an alias, since the brew of ohmyzsh and bash-it are different.
  • Keep the others where they are

Again, I have no strong opinion either way. Just looking to help out where I can.

@RobLoach RobLoach marked this pull request as draft September 6, 2024 00:26
@akinomyoga
Copy link
Contributor

Didn't know parity was desired, thanks for the explanation. Does this sound like a good path forwards?

  • Move our Chezmoi to aliases, since neither ohmyzsh or bash-it implement it as a plugin.
  • Move our brew plugin to an alias, since the brew of ohmyzsh and bash-it are different.
  • Keep the others where they are

Yes, that is what I'm thinking (if we are going to resolve these).

Anyway, I probably won't merge this PR soon. As explained above, destructive changes should be performed properly. At least we would need to do that when we increment the major version of OMB with other destructive changes.

@RobLoach RobLoach marked this pull request as ready for review September 8, 2024 09:59
@akinomyoga
Copy link
Contributor

Thank you! LGTM for now. We'll keep this PR until we perform the next major update.

@RobLoach
Copy link
Contributor Author

RobLoach commented Sep 9, 2024

Thanks, brought these change into a different branch at #609

@RobLoach RobLoach closed this Sep 9, 2024
@akinomyoga
Copy link
Contributor

Thank you for continuing to pay attention to the PR, but may I ask about your intent? It was just fine with this PR, which contains the related discussion.

@RobLoach
Copy link
Contributor Author

RobLoach commented Sep 9, 2024

I had accidently opened the branch off of master instead of creating a branch. We can keep this one instead. I'll just have to remember not to mess with my fork's master.

@akinomyoga
Copy link
Contributor

Ah, OK. Makes sense. Then, let's leave a reference to here in #609.

@akinomyoga akinomyoga removed the omb-next This issue has been fixed in the next oh_my_bash upgrade runs. label Sep 9, 2024
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.

2 participants