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

Call prompt as a single function #68

Merged
merged 1 commit into from
Dec 11, 2018
Merged

Call prompt as a single function #68

merged 1 commit into from
Dec 11, 2018

Conversation

andreiborisov
Copy link
Collaborator

@andreiborisov andreiborisov commented Apr 14, 2018

Reimplement workaround for truncated strings (#49) using single function as currently recommended in fish-shell/fish-shell#904 (comment) instead of event handler.

Update first line of the prompt when moving through directory history with "Alt + ←" and "Alt + →" (#66).

Also resolve #62 and #67.

@andreiborisov
Copy link
Collaborator Author

I'm using default fish prompt_pwd when first line is too long, but we can do this in more fancier ways similar to https://github.com/sindresorhus/pure and try to minimise current directory in several increasingly strict steps.

@andreiborisov
Copy link
Collaborator Author

Re-comitted to fix Travis tests as suggested by #70 (comment)

@boyeborg
Copy link
Contributor

@schrodincat I think the reason why the travis build fails is due to the dist: xenial option within the travis configuration. See #71

@rafaelrinaldi
Copy link
Collaborator

@schrodincat Could you please update your local version of pure and confirm if the issue still persists?

cc @boyeborg

@edouard-lopez
Copy link
Member

edouard-lopez commented Sep 10, 2018

@schrodincat test are green, could you resolve the conflict so we can move forward with your PR?

If you can add tests it's even better! 🎉

@andreiborisov
Copy link
Collaborator Author

Will fix this tomorrow, sorry for the delay😔

@andreiborisov
Copy link
Collaborator Author

andreiborisov commented Sep 10, 2018

Manual install script was a little bit broken (#81), but after fixing it, I haven't noticed any problems whatsoever. It seems, merge conflict was due to some spaces or other nonsense.

I didn't add any tests yet, though.

@rafaelrinaldi
Copy link
Collaborator

I appreciate your efforts here @schrodincat.

@andreiborisov
Copy link
Collaborator Author

Seems fine even after recent changes. @rafaelrinaldi feel free to let me know, if something need to be changed.
I’m also interested in implementing Git async functionality.

Sent with GitHawk

Copy link
Member

@edouard-lopez edouard-lopez left a comment

Choose a reason for hiding this comment

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

Thanks for the many PRs!

I added some feedback in order to improve our code 👍

fish_prompt.fish Outdated Show resolved Hide resolved
fish_prompt.fish Outdated Show resolved Hide resolved
fish_prompt.fish Outdated Show resolved Hide resolved
@andreiborisov
Copy link
Collaborator Author

@edouard-lopez I’ll look into it and fix it at Tuesday. Thanks for the review🌚

Sent with GitHawk

@andreiborisov
Copy link
Collaborator Author

I'm terribly sorry for such long delay. I've refactored my PR with feedback from @edouard-lopez in mind.
I've tried to be as generic as possible so code can be more easily maintained. Basically, I've divided pre prompt to elements each with three components: preferred delimiter before element, styling and text itself. This way we can easily count symbols so we won't exceed limit of columns in terminal. Also actual relative position of elements defined right before prompt printing, which will help if we'll want to incorporate more structure altering settings.

@andreiborisov
Copy link
Collaborator Author

This approach also fixes multiple spaces before git arrows

Copy link
Member

@edouard-lopez edouard-lopez left a comment

Choose a reason for hiding this comment

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

@schrodincat Thanks for the work ❤️

Would you bear with me to ask for a bit more work?

As the file starts to be pretty big, I provided some guidance to split the file in smaller functions/files that we can include using source.
This will help other contribute as files will be smaller thus less scary and allow us to tests each file/functions.

If my comment are not clear enough ping me, we can discuss together

fish_prompt.fish Outdated Show resolved Hide resolved
fish_prompt.fish Outdated Show resolved Hide resolved
fish_prompt.fish Outdated Show resolved Hide resolved
fish_prompt.fish Outdated Show resolved Hide resolved
fish_prompt.fish Outdated Show resolved Hide resolved
@edouard-lopez
Copy link
Member

@schrodincat could you allow changes on your PR so I can resolve conflict by rebasing it on master and help with the work?

@andreiborisov
Copy link
Collaborator Author

andreiborisov commented Dec 10, 2018

@schrodincat could you allow changes on your PR so I can resolve conflict by rebasing it on master and help with the work?

They're already allowed

@andreiborisov
Copy link
Collaborator Author

@edouard-lopez I'm working on the PR right now, please, wait, before making changes

@andreiborisov
Copy link
Collaborator Author

Phew, I hope, this time everything is dandy😅

Reimplement workaround for truncated strings (#49) using single function as currently recommended in fish-shell/fish-shell#904 (comment) instead of event handler.

Update first line of the prompt when moving through directory history with "Alt + ←" and "Alt + →" (#66).

Also resolve #62 and #67.

Refactor code into smaller functions.
@edouard-lopez
Copy link
Member

Awesome work! ❤️

Now there is plenty of things we can tests and thanks to you it will be a lot easier to do so and prevent further bug 👏

@edouard-lopez edouard-lopez merged commit b09bfdc into pure-fish:master Dec 11, 2018
@edouard-lopez edouard-lopez mentioned this pull request Dec 11, 2018
23 tasks
@edouard-lopez edouard-lopez modified the milestone: v2.0.0 Jan 9, 2019
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.

Working Directory not printed on first run
5 participants