Skip to content

fix(#321): refactor add_to_path + sep. OS logic#322

Merged
MordechaiHadad merged 6 commits into
MordechaiHadad:masterfrom
MrDwarf7:fix/non-interactive-setup
Sep 29, 2025
Merged

fix(#321): refactor add_to_path + sep. OS logic#322
MordechaiHadad merged 6 commits into
MordechaiHadad:masterfrom
MrDwarf7:fix/non-interactive-setup

Conversation

@MrDwarf7

Copy link
Copy Markdown
Contributor

Summary

Closes #321

Refactor add_to_path for handling non-interactive shells and separate OS logic to compile time
guarded functions.

Changes are mostly the addition of a guard check if we're interactive on either stdout/stderr, wrapping prompt with tokio::time::timeout, and moving the temp config to use a RefCell over cloning.

Testing

Currently requires end-user testing, will have to further refactor for mod_tests and proper testing between unix/windows OS's.

@MrDwarf7 MrDwarf7 force-pushed the fix/non-interactive-setup branch 3 times, most recently from 8c232d2 to 1f5d85e Compare August 28, 2025 10:26
@MordechaiHadad

Copy link
Copy Markdown
Owner

PR overall looks good, but i am not sure automatically modifying in non interactive shells, should we?

@MrDwarf7 MrDwarf7 force-pushed the fix/non-interactive-setup branch from 1f5d85e to d614ef2 Compare September 6, 2025 02:50
@MrDwarf7

MrDwarf7 commented Sep 6, 2025

Copy link
Copy Markdown
Contributor Author

@MordechaiHadad I'm from the standpoint that if a User is wanting to install something, their expectation would be that it gets added to path and becomes usable straight away. (See - Principle of least astonishment )

With how you've decided to do it too, via a self-contained shim for each major shell (env.fish/env.sh) and not a direct text/line addition to the config.fish/.bashrc I believe it's reasonable to do this automatically. Perhaps I add an additional message to the output stating what we actually did? Would allow the user to go and remove the shim/rename it so it's not picked up; effectively reverting the function call.

I also would make the (reasonably) safe assumption that most people doing fully non-interactive setup's are likely in the realm of devops, fresh-setup and are familiar with the tool already or at bare min. are SSH'ing which; atleast for 2 of them- require a reasonable level of comfort or competency with shell commands/tooling.
Take the referenced issue for the PR as a great example of this.

@MrDwarf7 MrDwarf7 force-pushed the fix/non-interactive-setup branch from b44099a to 01a540b Compare September 6, 2025 07:16
@MrDwarf7

MrDwarf7 commented Sep 6, 2025

Copy link
Copy Markdown
Contributor Author

Changes in those recent commits are:

  • That's now been re-based on your current up-to-date master branch,
  • I've updated tracing as it apparently had a security issue (see: #f590145d commit),
  • Split the larger Unix function up to better handle the Fish vs. Bash/Zsh differences, removing duplicate code in the process and
  • Also added tests to the use_handler functions I've updated/refactored or changed.
  • One of the tests complained about 'unused type alias' so I've put them behind a unix specific cfg flag and squashed it.

@MrDwarf7 MrDwarf7 force-pushed the fix/non-interactive-setup branch from dd2a644 to e328788 Compare September 6, 2025 07:29
@MrDwarf7

MrDwarf7 commented Sep 6, 2025

Copy link
Copy Markdown
Contributor Author

The tracing update/security vulnerability patch should also unblock #320 as well

Refactor add_to_path for handling non-interactive
shells and separate OS logic to compile time
guarded functions.
Deps update addresses security vulnerabilities.

Relevant link:
[RUSTSEC-2025-0055](https://rustsec.org/advisories/RUSTSEC-2025-0055)
Use smaller functions for readability and Err
recovery.
@MrDwarf7 MrDwarf7 force-pushed the fix/non-interactive-setup branch from e328788 to 9b00158 Compare September 23, 2025 17:16
@MrDwarf7

Copy link
Copy Markdown
Contributor Author

This has been rebased over master

@MordechaiHadad MordechaiHadad merged commit 42f785e into MordechaiHadad:master Sep 29, 2025
23 checks passed
@MrDwarf7 MrDwarf7 deleted the fix/non-interactive-setup branch September 29, 2025 10:24
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.

Error when running bob in noninteractive

2 participants