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

Attempt to make this a fish plugin #117

Merged
merged 4 commits into from
Oct 11, 2020
Merged

Attempt to make this a fish plugin #117

merged 4 commits into from
Oct 11, 2020

Conversation

folliehiyuki
Copy link
Contributor

@folliehiyuki folliehiyuki commented Oct 10, 2020

Check list

  • I have performed a self-review of my code
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation

Description

Related to issue #116.
I've only moved things around. It works flawlessly with Fisher but has some problems with OhMyFish. Local variables like set forgit_pager "$FORGIT_PAGER" aren't sourced by OhMyFish.
We can work around this by making them global, or create a symlink in install hook. I will address this in OhMyFish, since I don't know whether it is a bug.

Type of change

  • Bug fix
  • New feature
  • Refactor
  • Breaking change
  • Documentation change

Test environment

  • Shell
    • bash
    • zsh
    • fish
  • OS
    • Linux
    • Mac OS X
    • Windows
    • Others:

@cjappl
Copy link
Collaborator

cjappl commented Oct 11, 2020

Looking good so far. Definitely willing to give this a shot!

Will you please update the CI job to run the new "main" file instead of the old one? That is why the checks are failing.

That file is ".github/workflows/ci.yaml", change the line that looks like this:

- name: Test fish run: fish forgit.plugin.fish

to run the file in conf.d and make sure the checks are passing, then I'll merge

@cjappl
Copy link
Collaborator

cjappl commented Oct 11, 2020

and it's great to start with one, we can worry about OhMyFish next, this is a valuable step for now :)

@cjappl
Copy link
Collaborator

cjappl commented Oct 11, 2020

Oh, one more request. Please update the readme so that new users can "try out" the plugin without downloading the source. That line just needs a file location update. Also, please add a section for how to install this plugin using Fisher

@cjappl
Copy link
Collaborator

cjappl commented Oct 11, 2020

I don't see the readme update in this latest commit

@folliehiyuki
Copy link
Contributor Author

Sorry @cjappl I was kinda slow. Does it look alright?

@cjappl
Copy link
Collaborator

cjappl commented Oct 11, 2020

Awesome, let's give this a try.

Thanks for your help on this issue. Much appreciated : )

@cjappl cjappl merged commit 2ac780c into wfxr:master Oct 11, 2020
@folliehiyuki
Copy link
Contributor Author

folliehiyuki commented Oct 11, 2020

Ah shoot @cjappl I forgot we need to copy (or symlink) the functions directory in the manual installation or it will not work.

And my change to the "try out" section seems wrong. I have no idea about the Github shortlink tbh.

@cjappl
Copy link
Collaborator

cjappl commented Oct 11, 2020

Reverted it for now while we sort out these issues! thanks for catching them

@cjappl
Copy link
Collaborator

cjappl commented Oct 11, 2020

@wfxr how did you create those short links to begin with, and how would we update git.io/forgit.fish to point to the new file once this is submitted again?

@folliehiyuki
Copy link
Contributor Author

folliehiyuki commented Oct 11, 2020

I can't think of an easier way to install the plugin manually @cjappl .

Same goes for the shortlink. It has to curl all the function files too.

@cjappl
Copy link
Collaborator

cjappl commented Oct 11, 2020

Damn, that's a good point.

I wonder how many of our users use that shortcut versus would use it if it were available on fisher/OhMyFish?

Do you have a plugin that you like to use that you've installed with fisher/OhMyFish that we could look at their repo? maybe they have some hints for us

@folliehiyuki
Copy link
Contributor Author

I use some that are presented in this repo.

And here are the official OhMyFish plugins. Fisher is compatible with OhMyFish plugins too.

@wfxr
Copy link
Owner

wfxr commented Oct 12, 2020

@cjappl

how did you create those short links to begin with, and how would we update git.io/forgit.fish to point to the new file once this is submitted again?

It's a shorten URL created by git.io.

@AoHiyuki
Is there a way to add support for fisher/omf without splitting file?

@folliehiyuki
Copy link
Contributor Author

Is there a way to add support for fisher/omf without splitting file?

@wfxr there is. We can combine all the files into 1 file like before, and put it inside conf.d directory. Fisher will work, but the same problem remains with OhMyFish.

@cjappl
Copy link
Collaborator

cjappl commented Oct 12, 2020

Let's give that a try for the first step, seems like low impact change that can get us functionality on one of the managers at least!

@folliehiyuki
Copy link
Contributor Author

Ok. Then we'll go with that path.

Do you want me to make the change, and commit it in a new pull request? Since all we need to do is few readme changes and rearrange the file, it will be quick.

@wfxr
Copy link
Owner

wfxr commented Oct 13, 2020

@AoHiyuki Yes. You can just submit a new pr. I will help update the readme.

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.

None yet

3 participants