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

Add Fundle install support #30

Closed
wants to merge 33 commits into from
Closed

Add Fundle install support #30

wants to merge 33 commits into from

Conversation

SirWrexes
Copy link
Contributor

Hey there,

I'm not a fan of installing plugins straight into where I put my own configs.
Nonetheless, I've been using Fisher for a while until, recently, jorgebucaran/fisher#651 broke everything.
I kept getting annoying warnings telling me to delete my plugin files in order to update them blablabla... but I digress.

I switched to Fundle, which works more elegantly IMO by placing plugins in a dedicated directory tree then sourcing everything.

Luckily, there are several ways of checking if a user has Fundle, and one of them is querying functions too see if __fundle_plugins_dir exists, which will then serve to tell us where Gitnow has been installed.
I quickly wrote a fix with this method, while retaining the pretty handy variables you created.

Note that fish_config could also be set using the value from the standard __fish_config_dir variable.

Have a good day ! 👋

@joseluisq joseluisq self-assigned this Mar 4, 2021
@joseluisq joseluisq added the enhancement New feature or request label Mar 4, 2021
@joseluisq
Copy link
Owner

joseluisq commented Mar 4, 2021

I switched to Fundle, which works more elegantly IMO by placing plugins in a dedicated directory tree then sourcing everything.

Luckily, there are several ways of checking if a user has Fundle, and one of them is querying functions too see if __fundle_plugins_dir exists, which will then serve to tell us where Gitnow has been installed.
I quickly wrote a fix with this method, while retaining the pretty handy variables you created.

IMO your PR looks more like a feature rather than a fix since as I understood you want to add Fundle support. Right?
If so please update the title to "Add Fundle install support" or something.

Note that fish_config could also be set using the value from the standard __fish_config_dir variable.

Yeah, you could send a new PR introducing that change if you want. $XDG_CONFIG_HOME is kind of restricted.

Copy link
Owner

@joseluisq joseluisq left a comment

Choose a reason for hiding this comment

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

It would be also great if you can share the steps to test Gitnow with Fundle.

@@ -8,11 +8,19 @@ set -q fish_functions; or set -g fish_functions "$fish_config/functions"
set -q fish_completions; or set -g fish_completions "$fish_config/completions"
set -q GITNOW_CONFIG_FILE; or set -g GITNOW_CONFIG_FILE ~/.gitnow

set -g gitnow_version 2.6.0
set -g gitnow_version 2.7.0
Copy link
Owner

Choose a reason for hiding this comment

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

Why that change? Are you using the latest master? Can you sync your PR branch please?
https://github.com/joseluisq/gitnow/blob/master/conf.d/gitnow_config.fish#L11

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh my bad. I simply pulled from master when forking, then made the fundle-compat branch after editting the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait, on master it's 2.6 too.
I honestly don't know what happened there, that is quite odd. I'll revert it.

Copy link
Owner

Choose a reason for hiding this comment

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

Basically you need to sync your fork first.

@joseluisq
Copy link
Owner

And probably you also want to update the Install section of README with the new install method. https://github.com/joseluisq/gitnow#install

@SirWrexes SirWrexes changed the title fix: Be fundle aware when trying to source configs Add Fundle install support Mar 4, 2021
@SirWrexes
Copy link
Contributor Author

I'm working on adding the other things like editing the README etc.
So, what version number should I keep ? 2.7.0 I assume ?

@joseluisq
Copy link
Owner

So, what version number should I keep ? 2.7.0 I assume ?

Yes, Just let it as it is as current Gitnow master.

SirWrexes and others added 10 commits March 4, 2021 17:17
Config file parsing (.gitnow) was written completely and as result it
boosts performance startup of GitNow up to 62% faster avg.

Stats:

Old parser (v2.6.0):
Executed in  223,04 millis    fish           external
   usr time  209,69 millis  636,00 micros  209,06 millis
   sys time   31,97 millis  175,00 micros   31,80 millis

New parser:
Executed in   84,62 millis    fish           external
   usr time   63,87 millis    0,00 micros   63,87 millis
   sys time   24,04 millis  921,00 micros   23,12 millis
it limits max number of commits to 80 by default
@SirWrexes
Copy link
Contributor Author

SirWrexes commented Mar 4, 2021

Alright.

I have added the installation instructions to README.md, and squashed that plus the edits to gitnow_config.fish into one feat: commit.

I also took time to add a disclaimer about 2.7.0 not supporting Fundle yet in the "Latest stable" installation instructions. Since Fundle's README isn't very clear about how to get a particular version of a plugin and it's not extremely user friendly, I thought it was better having the installation method + a note rather than nothing. It will save you some time later too I guess.

Synchronising my fork in between two commits made squashing take longer than expected but everything seems to be in order and working fine.
I tried it out by copying my fork checked out on fundle-compat in my Fundle plugin dir and nothing weird came up.

I'll push the squashed commits.

@SirWrexes
Copy link
Contributor Author

Wow seeing the whole history come up in there because of the sync issue sure is a scary thing.
My apologies. I'm kinda new to the PR thing.

@joseluisq
Copy link
Owner

Wow seeing the whole history come up in there because of the sync issue sure is a scary thing.
My apologies. I'm kinda new to the PR thing.

Understand, but unfortunately the history is a bit messy I recommend do a new PR with just your commit changes.

@joseluisq joseluisq closed this Mar 4, 2021
@joseluisq
Copy link
Owner

Feel free to sync your fork or re-fork Gitnow again and open a new PR again.

@SirWrexes
Copy link
Contributor Author

I totally understand.
Will do !

@SirWrexes SirWrexes deleted the fundle-compat branch March 4, 2021 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants