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

Tide tests assume that tide is installed in $__fish_config_dir #62

Closed
vamega opened this issue Dec 30, 2020 · 5 comments · Fixed by #65
Closed

Tide tests assume that tide is installed in $__fish_config_dir #62

vamega opened this issue Dec 30, 2020 · 5 comments · Fixed by #65
Labels
🐛 bug Something isn't working

Comments

@vamega
Copy link
Contributor

vamega commented Dec 30, 2020

Describe the bug

Right now some code assumes that tide is installed under $__fish_config_dir.

If installed via fisher it is possible that tide is actually under a different directory root, through the use of $fisher_path. See this fisher issue for more details.

Steps to reproduce

Don't really know the best way to reproduce this, since I'm not familiar with how to run the tests. Should the tests be run without Tide installed, or is it okay to run them in a shell where Tide is already running?

  1. Set $fisher_path to ~/.config/fish/fisher
  2. Place the snipped from here in ~/config/.fish/conf.d/fisher.fish
  3. Install fisher
  4. Install tide
  5. See error about sourcing _tide_sub_configure.fish

I don't the setup for the tests (Do you run the tests after you've installed tide, or separately?), but I think the test above should fail.

Expected behavior

The tests should not assume an installation path, but instead discover it's installation path, and identify paths under that root.

Environment (please complete the following information)

  • Operating System: Arch Linux
  • Output of tide bug-report:
fish, version 3.1.2
tide, version 3.0.0
xterm-256color

Additional context

I had a fork that was addressing some of these issues using a universal variable, but it seems like you've removed almost all instances of this assumption. This test case is the only one left, but it seems like you've mostly eliminated the issue with a different approach, so if you could fix this one last test I think you'll have completely removed the issue.

@vamega vamega added the 🐛 bug Something isn't working label Dec 30, 2020
@jorgebucaran
Copy link
Contributor

Just a thought. Fisher could pass the current path to *_install events so that we don't need to use $__fish_config_dir or $fisher_path here:

function _tide_init_install --on-event _tide_init_install
_set_immutable _tide_color_dark_blue 0087AF
_set_immutable _tide_color_dark_green 5FAF00
_set_immutable _tide_color_gold D7AF00
_set_immutable _tide_color_green 5FD700
_set_immutable _tide_color_light_blue 00AFFF
_set_immutable _tide_dir "$__fish_config_dir/functions/tide"
_set_immutable VIRTUAL_ENV_DISABLE_PROMPT true
set -U _tide_var_list
source $__fish_config_dir/functions/_tide_sub_configure.fish

Or maybe use status filename to get the current file name and extract the directory from there.

@vamega
Copy link
Contributor Author

vamega commented Dec 30, 2020

@jorgebucaran I used the status filename approach in my fork over here.

What do you see as the pro's and con's of having fisher pass the path, vs using status filename?

@IlanCosman
Copy link
Owner

Not 20 hours ago I thought of this myself, tried using $fisher_path etc, but it wasn't working at all for me because I didn't have the snippet from jorgebucaran/fisher#640. So I gave up and decided to wait for someone to report it 😂

I'd say if there's a reasonable way for plugins to deal with something themselves without spending too much code, it shouldn't be in Fisher. That's how I feel in this case.

@vamega Thanks for opening this issue 😄 When I was working on this, I used functions --details to get the paths of the functions. I think that's a better approach because it doesn't use any non-builtins like dirname. Would you mind making a PR using functions --details?

@IlanCosman
Copy link
Owner

IlanCosman commented Dec 30, 2020

Woops, functions --details doesn't matter here 😅Not sure what I was talking about.

What I'd actually like is replacing the dirnames with fish builtins like string replace. We did the same with basename.

Also, no need for two variables, just one called _tide_dir is fine.

@jorgebucaran
Copy link
Contributor

@IlanCosman I misreported this because I was thinking of an older Fisher where $fisher_path was always global. Now that it isn't, using $fisher_path won't work. To grab the real path we are going to need status filename.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🐛 bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants