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

fix(docker_context): ignore unix domain socket path from Docker Context #5616

Merged
merged 2 commits into from Dec 17, 2023

Conversation

ananta
Copy link
Contributor

@ananta ananta commented Dec 4, 2023

fix(modules): ignore unix domain socket path from Docker Context

Description

Implemented functionality to slice long Unix domain socket paths in Docker Context within the terminal prompt. This optimization ensures the prompt remains concise and user-friendly, especially in environments where Unix domain socket paths are lengthy.

Motivation and Context

This enhancement addresses the issue of excessively long terminal prompts when Unix domain sockets are used in Docker Contexts. The change is aimed at improving the user experience by maintaining a manageable prompt length. Relevant issue:
Closes #5548

Screenshots (if appropriate):

Before:
image
After:
image

How Has This Been Tested?

  • I have tested using MacOS

Checklist:

  • I have updated the tests accordingly.

Comment on lines 75 to 79
"context" => Some(Ok(if ctx.contains("unix://") {
""
} else {
ctx.as_str()
})),
Copy link
Member

@davidkna davidkna Dec 5, 2023

Choose a reason for hiding this comment

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

I would prefer handling it like this:

Suggested change
"context" => Some(Ok(if ctx.contains("unix://") {
""
} else {
ctx.as_str()
})),
"context" if !ctx.starts_with("unix://") => Some(Ok(ctx.as_str())),

Copy link
Member

Choose a reason for hiding this comment

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

On the other hand, it might make sense to avoid displaying the module at all and returning None like when the value is default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for reviewing the PR 😄 . I've updated the codebase accordingly. Let me know if you have any more suggestions!

Copy link
Member

Choose a reason for hiding this comment

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

I thought a bit about this, and please do move the ctx.starts_with("unix://") above and combine it with the ctx == "default" check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure @davidkna,
But, if we merge the check for the "default" context with the current condition, the via 🐳 text will be omitted from our prompt when a Docker context is present. Is this an acceptable outcome?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidkna Refactored!
I've squashed the irrelevant commits and retained only the first two for clarity. This ensures that if we need to revert in the future, the necessary changes are readily accessible. Thank you for reviewing the PR!

@andytom andytom changed the title fix(modules): ignore unix domain socket path from Docker Context fix(docker_context): ignore unix domain socket path from Docker Context Dec 9, 2023
@ananta ananta force-pushed the skip-unix-domain-socket-path branch from 2ad4f87 to ddf38b0 Compare December 16, 2023 14:30
@andytom andytom merged commit a910e09 into starship:master Dec 17, 2023
17 checks passed
@andytom
Copy link
Member

andytom commented Dec 17, 2023

Thank you for your contribution @ananta and thank you for reviewing @davidkna.

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.

DOCKER_HOST contains unix:// should be skipped
3 participants