Skip to content

activate.fish: update fish major version check #2891

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

r5d
Copy link

@r5d r5d commented May 24, 2025

Use fish's string sub command to get fish's major version from
$FISH_VERSION.

The head program in OpenBSD does not have the -c flag.

Addresses #2892.

gaborbernat
gaborbernat previously approved these changes May 27, 2025
@gaborbernat gaborbernat enabled auto-merge May 27, 2025 14:42
Use fish's [`string sub`][ss] command to get fish's major version from
`$FISH_VERSION`.

The [`head`][head] program in OpenBSD does not have the `-c` flag.

Addresses pypa#2892.

[head]: https://man.openbsd.org/head
[ss]: https://fishshell.com/docs/current/cmds/string.html#sub-subcommand
auto-merge was automatically disabled May 28, 2025 23:07

Head branch was pushed to by a user without write access

@r5d r5d force-pushed the activate-fish-openbsd branch from 24f90bb to 55caf36 Compare May 28, 2025 23:07
@r5d
Copy link
Author

r5d commented Jun 21, 2025

@gaborbernat some tests are failing and it does not seem to be related to the changes in this PR. Are you able to look into it and possibly get this merged?

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

Let's not worry about other tests until you add a test to validate your change.

Add `test_fish_path` to verify that the PATH is reverted to the
original PATH after virtualenv deactivation. This verification
confirms that the `_OLD_VIRTUAL_PATH` was correctly set in fish during
the activation of the virtualenv.
@r5d
Copy link
Author

r5d commented Jun 28, 2025

Let's not worry about other tests until you add a test to validate your change.

@gaborbernat

Added a test in test_fish.py to verify that the PATH is reverted to
the original PATH after virtualenv deactivation. This verification
confirms that the _OLD_VIRTUAL_PATH was correctly set in fish during
the activation of the virtualenv.

See if it looks good.



@pytest.mark.skipif(IS_WIN, reason="we have not setup fish in CI yet")
def test_fish_path(activation_tester_class, activation_tester, monkeypatch, tmp_path):
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason we can't include this into test_fish?

Copy link
Author

Choose a reason for hiding this comment

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

@gaborbernat I've moved the test into test_fish.

Move test `test_fish_path` into `test_fish`. Rename the class that
verifies the preservation of original `PATH` after virtualenv
deactivation to `FishPath`.
@@ -20,4 +20,25 @@ def __init__(self, session) -> None:
def print_prompt(self):
return "fish_prompt"

class FishPath(activation_tester_class):
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be merged into the Fish class, we don't need two classes here 🤔

Copy link
Author

Choose a reason for hiding this comment

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

@gaborbernat moved the test into the Fish class.

Merge test in `FishPath` into `Fish`.
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.

2 participants