oh-my-fish / oh-my-fish-legacy

Oh My Fish!
MIT License
13 stars 2 forks source link

Gives back a way to persist and update installed packages (dotfiles) #564

Closed derekstavis closed 9 years ago

derekstavis commented 9 years ago

This PR makes omf cli install from and update a central file declaring installed plugins and themes.

The file is located at $OMF_CONFIG/bundle, and is a user-editable text file in the following format:

package foreign-env
theme agnoster copy

Blank lines are ignored as do lines that don't start with package or theme.

What it changes from current cli:

On clean installs where $OMF_CONFIG exists, the installer will satisfy the bundle before reloading back into fish.

ghost commented 9 years ago

You know, there just can't be races in the community. Don't get me wrong, if you really want to do this feature, sure, go ahead and by all means, but I assumed I was going to take care of it. This is not a competition, but having believed the previous from out communication over gitter I went ahead and put a decent amount of work two days ago day despite being busy with other work, and unfortunately can't only really focus on this in a few days.

I am not even mad or disappointed or anything, I'm, perhaps "burned out" of fighting over all this stuff /sigh. It's funny.

derekstavis commented 9 years ago

@bucaran I've put a massive amount of work on this. It's working, it was exhaustively tested by me and it's commits are very logical and organized. I think we need to solve this problem ASAP, and implementing by myself something that just works seemed the best thing to do.

ghost commented 9 years ago

@derekstavis There's no rush that can't wait a few days, not even a week and I'm on the same boat.

Don't take it personally, I just thought it was funny how you went ahead and did what you wanted to do anyway.

sheldon commented 9 years ago

Hey @derekstavis

I kind of like that install without arguments offers what those arguments could be. Backwards compatibility goes a long way to keeping users happy. :smile:

What about making update behave this way in addition to updating packages instead? It seems more natural to me that update would install missing packages from a registry.

As the output from update without arguments is unlikely to be parsed, whereas install without arguments is the primary way to list available packages currently, I think it's a safer change to previous behaviour too.

Another option: a sync command instead.

bpinto commented 9 years ago

Until last week, omf install without arguments would install all packages on your config file, so I think this is actually solving backwards compatibility issues for 99% of the users.

My experience working with other CLI tools (e.g brew, bundler) also lead me to think that omf install would install many packages instead of listing the packages.

I don't know any tool where an update will actually call install, do you have an example?

sheldon commented 9 years ago

Nothing comes to mind. Funnily enough, before last week I hadn't used this before :)

I'm all for sticking with the way things were before. Fixing regression ftw.

derekstavis commented 9 years ago

omf update --bundle is what I was thinking for the future PR I'm planning on for implementing version control of bundle items.

bpinto commented 9 years ago

LFTM

@bucaran @bobthecow opinions?

ghost commented 9 years ago

I don't know

I say :+1: and then let's fix the mess later. I will have time to start testing this later today.

scorphus commented 9 years ago

I like this approach. And I'd add that I like the --sync option, but some may see it as superfluous, I guess. I also miss a way to list all the available packages.

derekstavis commented 9 years ago

I've rebased it based on master @bpinto. @scorphus I will follow with a PR for allowing a better list command.

scorphus commented 9 years ago

:+1:

bobthecow commented 9 years ago

If we're going to go with something simple, why not two files, packages and themes each of which have one entry per line with the name of the package or theme? Having something that we have to parse just seems like overkill for this. Having a third concept (themes, packages and now bundles) seems like overkill as well.

derekstavis commented 9 years ago

Great point @bobthecow. The problem I had with packages and themes approach is that currently we can have directories with this names inside $OMF_CONFIG. Since we are going to rename pkg to plugin this name clash can happen. That's why I preferred a single file rather than different files, which could make more sense, and avoid the need to parse it (although the parser is very simple).

bpinto commented 9 years ago

I like the current concept of bundle and theme. They have different purposes.

bundle: Themes and packages that you have installed and loaded. theme: Current loaded theme.

We might want to remove the theme file in the future, but it might be a bigger discussion.

derekstavis commented 9 years ago

I like the fact that the bundle can contain more than one theme. I often swap themes on my different machines during the day. Sometimes the theme is too heavy for my little embedded machines, and while on heavy terminal usage I swap to simple theme (recalculating git every enter isn't great on them) and when I need git stuff I change to agnoster of bobthefish.

bobthecow commented 9 years ago

Wasn't there one of the pull requests floating around that moved the theme and revision files into a subdirectory to make it more obvious that it was a programmatically generated one rather than a user file? Seems that would solve this issue.

On Aug 30, 2015, at 6:45 PM, Derek Willian Stavis notifications@github.com wrote:

Great point @bobthecow. The problem I had with packages and themes approach is that currently we can have directories with this names inside $OMF_CONFIG. As we are going to rename package to plugin the same could happens. That's why I preferred a single file rather than different files, which could make more sense, and avoid the need to parse it (although the parser is very simple).

— Reply to this email directly or view it on GitHub.

bpinto commented 9 years ago

Hum... let me think:

We have three files:

I don't know, but I'd rather try to remove both revision and theme file instead of moving all three to a new directory.

derekstavis commented 9 years ago

theme file is OK for me for dotfiles usage. It's great to clean install omf keeping $OMF_CONFIG and see how everything gets back as it was.

derekstavis commented 9 years ago

I just rebased this PR to master for anyone who wants to test it.

scorphus commented 9 years ago

@bucaran Did you have time to test it?

@all What are the major issues you see with this approach?

scorphus commented 9 years ago

@derekstavis What if I already have a bunch of packages/theme(s) installed, like is the case for most of us?

This is what I did:

omf.list_installed_packages | awk '{print "package " $1}' > $OMF_CONFIG/bundle
omf.list_installed_themes | awk '{print "theme " $1}' >> $OMF_CONFIG/bundle
derekstavis commented 9 years ago

Yes, this is a easy way to sync current state! :+1:

scorphus commented 9 years ago

But that's just a makeshift-that-solved-my-problem thing.

scorphus commented 9 years ago

I actually expected either omf install or omf update would do this favor to me.

ghost commented 9 years ago

Not yet. I will later today.

Another suggestion, what about just keeping a file with the plugin names and be done with this?

Users can then omf install (cat $OMF_CONFIG/plugins)

derekstavis commented 9 years ago

I think we don't want to go omf install (cat $OMF_CONFIG/plugins) way. This is such a bad cli. About keeping just a file with plugins forgets about multiple theme support.

ghost commented 9 years ago

There is no multiple theme support in the traditional sense. OMF installs your theme with omf theme name whenever you like. No need to keep a list of local and remote themes.

TBH I don't think the CLI is bad. This is shell business. If we wanted a more sophisticated CLI then we would use a higher level language.

I love the idea of simply omf theme name and use the theme I want without knowing whether the theme is already installed or not. It just works. We want a fast framework that gets out of the way right?

derekstavis commented 9 years ago

I love the idea of simply omf theme name

So simple that it tries to upgrade the theme everytime you change it.

We want a fast framework that gets out of the way right?

I think everyone wants a flexible and extensible framework done by everyone, not just by one.

ghost commented 9 years ago

We could totally remove the autoupdate behavior!

IMO this PR adds unneeded complexity. What's wrong with omf install (cat $OMF_CONFIG/plugins)?

derekstavis commented 9 years ago

Sorry. Your argument is really invalid. Look back to your PR again and then come and say this one adds complexity. The problem isn't with the PR, it's with your ego. You need to accept people ideas, not just want to impose yours and assume the project is yours (even it being now). This kind of behaviour is toxic, and that's why now omf is broken. And don't come talk to me about user experience when you were the first that used your persuasion and done a big mistake by letting all omf users fuck themselves.

ghost commented 9 years ago

The only main feature missing in Wahoo was this and I find it only useful when removing and reinstalling OMF :confused:

In fact, we could easily allow users to add their own commands to omf (or just write a plugin for that).

bpinto commented 9 years ago

@derekstavis Take it easy... We are again back to the same unrelated comments...

@all Things are not moving forward, most agreed this was good. So I'm using that to merge it. End of story.

ghost commented 9 years ago

@bpinto This move was too fast :confused:

bpinto commented 9 years ago

Topic is closed, please let's move on.

ghost commented 9 years ago

I will give it a shot and see.