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

Installation with user/repo syntax #824

Merged
merged 3 commits into from
Mar 26, 2022

Conversation

fdeitylink
Copy link
Contributor

Description

Allows installation from GitHub repositories with user/repo syntax. Shorthand for URL syntax, more or less, with GitHub as the forge.

Fixes #818

Environment report

Oh My Fish version:   7-35-g332ca07
OS type:              Linux
Fish version:         fish, version 3.1.2
Git version:          git version 2.25.1
Git core.autocrlf:    no
Checking for a sane environment...

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • New and existing tests pass locally with my changes

@fdeitylink fdeitylink requested a review from a team as a code owner March 3, 2021 19:47
@fdeitylink fdeitylink changed the title Install from name Installation with user/repo syntax Mar 3, 2021
@@ -107,7 +108,7 @@ Apply a theme. To list available themes, type `omf theme`. You can also [preview

#### `omf remove` _`<name>`_

Remove a theme or package.
Remove a theme or package. If a package was installed via `user/repo`, use `repo` for `name`.
Copy link
Member

Choose a reason for hiding this comment

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

Should we refer to packages installed this way as user/repo? Otherwise we have potential collisions as multiple users might have the same repo name, and repo names might collide with package names from the registry.

Thinking about it, maybe a good approach would be to install these as @user/repo in the filesystem, while still allowing them to be referred to as either user/repo or repo as long as there is no ambiguity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. I have to test if installing to $OMF_PATH/pkg/user/repo would break anything since packages are expected to be found in $OMF_PATH/pkg. Additionally, if we're going with that, I think we shouldn't use omf.packages.name to set a package name when installing via URL or user/repo syntax. In my opinion it creates a potentially confusing disparity between installation and uninstallation. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, yeah. I was thinking that @user/repo would keep the ones installed via this mechanism from colliding, but I guess we have a similar problem with packages installed by URL.

Copy link
Contributor Author

@fdeitylink fdeitylink Mar 9, 2021

Choose a reason for hiding this comment

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

I'm thinking the installation subdirectory could map basically 1 to 1 with the installation "name".

  • Install via name (i.e. something registered in packages-main): $OMF_PATH/pkg/name
  • Install via name/repo: $OMF_PATH/pkg/name/repo
  • Install via url: $OMF_PATH/pkg/url

This isn't wholly uncommon, prevents conflicts, and usually users shouldn't need to muck around in the pkg directory much anyway.

Copy link
Member

Choose a reason for hiding this comment

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

The separation of plugins in namespaces to avoid collisions seems like a good idea, but I think it shouldn't prevent this PR to be merged, as the functionality is already there — and so is the collision. I'm merging this one and opening an issue for when we'd like to improve pkg installation and mitigate collisions.

@scorphus scorphus added the HackIllinois Issues to be tackled by awesome hackers on HackIllinois 2019 label Oct 11, 2021
@scorphus scorphus merged commit 029a675 into oh-my-fish:master Mar 26, 2022
@scorphus
Copy link
Member

Many thanks, @fdeitylink! 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HackIllinois Issues to be tackled by awesome hackers on HackIllinois 2019
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: Assume github.com when no package is found
3 participants