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

activate command #283

Merged
merged 10 commits into from
Oct 8, 2022
Merged

activate command #283

merged 10 commits into from
Oct 8, 2022

Conversation

LoipesMas
Copy link
Contributor

Closes #12

Implements the activate command.
Pretty much just translated from poetry 😅

Adds expectrl dependency for pseudo-shell-session (shout-out to that crate, made this possible at all) and terminal_size to get terminal size ;P

Not perfect, but should be good enough for 90% of use-cases.

Should probably be tested on different systems (especially Windows, because I haven't tested that at all).

Let me know if you find any bugs or want anything changed!

@cnpryer
Copy link
Owner

cnpryer commented Oct 6, 2022

Hey thanks for this! I should be able to dive into this PR soon. If I can't get to it today I should be able to check it out tomorrow.

Copy link
Owner

@cnpryer cnpryer left a comment

Choose a reason for hiding this comment

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

Looks good! A couple comments. Otherwise I love it. I'm still adjusting to the dependency philosophy in the rust ecosystem. So adding stuff like terminal_size throws me off. But I think it makes sense for at least this PR.

Thanks so much for getting the activate command started!

Btw, #278 is merged. You'll need to rebase. It's a change we've been pretty eager to make. Sorry about any conflicts! Also #287 but I blame you for that one! 😉 Definitely going to use that more. Ty!

src/bin/huak/commands/activate.rs Outdated Show resolved Hide resolved
src/huak/env/venv.rs Outdated Show resolved Hide resolved
@cnpryer
Copy link
Owner

cnpryer commented Oct 7, 2022

Also I see you added the environment variable. If you're feeling up to it I'd love to add a test for the huak::ops::activate implementation to verify a venv can be activated using activate_project_venv.

@LoipesMas
Copy link
Contributor Author

adding stuff like terminal_size throws me off

It feels a bit weird to me as well. But it has some platform specific code so replicating that in our code would be too much imo. And it's pretty small.

@LoipesMas
Copy link
Contributor Author

Actually testing that whole function might be problematic, because it waits on user input. I could maybe extract those parts that can be tested and only test them, but that seems kinda backwards, at least IMO.
Any ideas?

@cnpryer
Copy link
Owner

cnpryer commented Oct 7, 2022

Hmm. Sounds tricky. It's not the end of the world if we can't test this as part of the PoC but definitely worth tagging the post-PoC chores tracking issue if that's the case.

I'm going to try to merge some other PRs, then I'll come back to this to see if I can help.

@cnpryer
Copy link
Owner

cnpryer commented Oct 7, 2022

FYI I'm thinking after tinkering with this earlier my terminal is broken.

huak on  master [$] is 📦 v0.0.5-alpha.2 via 🐍 v3.10.4 (.venv) via 🦀 v1.64.0 
❯ gh pr checkout 289
remote: Enumerating objects: 65, done.
remote: Counting objects: 100% (65/65), done.
remote: Compressing objects:  24% (6/2hauk exited with code ExitCode(unix_exit_status(1)): Error related to pseudo-terminal: IO error Resource temporarily unavailable (os error 35).
remote: Compre%                                                                                                                                                 

huak/resources/mock-project on  master [$] is 📦 v0.0.1 via 🐍 v3.10.4 (.venv) took 15h1m5s 
                                                                                             ❯ 

huak/resources/mock-project on  master [$] is 📦 v0.0.1 via 🐍 v3.10.4 (.venv) 
  git status                                                                    ❯ 
On branch master
                Your branch is up to date with 'origin/master'.

                                                               nothing to commit, working tree clean
                                                                                                    %                                                           

huak/resources/mock-project on  master [$] is 📦 v0.0.1 via 🐍 v3.10.4 (.venv) 
  cd ../../                                                                     ❯ 

huak on  master [$] is 📦 v0.0.5-alpha.2 via 🐍 v3.10.4 (.venv) via 🦀 v1.64.0 
                                                                                ❯

macOS Monterey 12.6
zsh from vscode
+ starship

@LoipesMas
Copy link
Contributor Author

It seems that expectrl returned an error IO error Resource temporarily unavailable (os error 35) and to be fair I'm not sure what that means. Probably stdin/stdout was unavailable, but why/how? And can we do something about it or is it an error in expectrl?

If you found a way to reliably reproduce it, that would help!

@cnpryer
Copy link
Owner

cnpryer commented Oct 8, 2022

I'll see if I can reproduce it today

Copy link
Owner

@cnpryer cnpryer left a comment

Choose a reason for hiding this comment

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

I couldn't reproduce that issue I had before, so with some final polishing I think we merge :)

src/huak/ops/activate.rs Show resolved Hide resolved
src/huak/utils/shell.rs Outdated Show resolved Hide resolved
@LoipesMas
Copy link
Contributor Author

I just found a bug: so I assumed that if we have Venv struct and it is not None, then we can assume that it exists in the filesystem. Turns out it's not true. Found it out by (accidentally) running huak activate in huak's root directory (while not having any .venvs there). It fails while trying to source the activation script (because it doesn't exist).
I think that's an issue with huak, either in design (i.e. Venv should check if it exists on the filesystem before being constructed) or documentation/naming (so it would convey that it can be "uninitialized")

For now I'll just add a check that the file exists.

@cnpryer
Copy link
Owner

cnpryer commented Oct 8, 2022

Makes sense! Yea there's some notes on this sort of stuff somewhere (if not in the pre 0.1.0 chores issue). I'd actually like to improve the design of project and venv structure altogether.

Id really like to refactor the two so that venv is a kind of env context that projects can be operated on within.

I'm on mobile but I'll edit this to tag the tracking issue if it's not already documented.

@cnpryer
Copy link
Owner

cnpryer commented Oct 8, 2022

Love it! Great PR. Thank you!

@cnpryer cnpryer merged commit d3fcea2 into cnpryer:master Oct 8, 2022
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.

Basic activate command
2 participants