🚀🐟 + 🌊 | Spacefish swim towards v1.0

Snuggle commented 6 years ago

Description

This pull request attempts to help push Spacefish towards complete feature parity with Spaceship with a 🌊

Motivation and Context

It aims to fix/implement the following issues: #20 #28 #29 #32 #34 #37 and help push Spacefish to the v1.0 milestone.

Types of changes

[x] Several new features (non-breaking changes which adds functionality)

Screenshots (if appropriate):

How Has This Been Tested?

[x] I have tested using MacOS
[x] I have tested using Linux

Default Pull Request Checklist:

[x] I have checked that no other PR duplicates mine
[x] My code follows the code style of this project.
[x] My change requires a change to the documentation.
[x] I have updated the documentation accordingly.
[x] I have updated the tests accordingly.

Personal Development Checklist:

[x] Create initial rough translation from Zsh to Fish.
[x] Write a lot of documentation.
[ ] Manually test Windows battery-acpi on a Windows laptop.
[x] Manually test pyenv section.
[x] Manually test background jobs section.
[x] Manually test time section.
[x] Manually test exit code section.
[x] Manually tested everything.
[x] Add automated tests.
[x] Automated tests complete without errors.
[x] Swim-to-v1.0 is ready to merge. 🚀🐟 + 🌊

matchai commented 6 years ago

Woah! Looks like some serious work is being done here! 👏👏👏

I would suggest breaking every new section into its own PR with its milestone set to 1.0.0 to make the reviewing merging process cleaner.

Looking forward to reaching 1.0.0! 🎉

Snuggle commented 6 years ago

To clarify, split each section relative to this branch, or from the master branch? A lot of the sections have been written in commit 41847d0ef2ccb77ddd23d2e42f77403efb9af6db, but need testing, documentation and possibly small fixes.

I'm also not able to set milestones on issues or pull requests, I think only maintainers/owners can do that. :yum:

matchai commented 6 years ago

I was thinking that we would have each section be its own PR relative to master, but if we're already far gone, it'd be something to keep in mind for next time. 😄

Snuggle commented 6 years ago

I've noticed quite a bit of the documentation is out of order, I'll shuffle things around to make it in the same order as the prompt, for ease-of-use. :slightly_smiling_face:

Snuggle commented 6 years ago

Heya! This is almost ready to merge, this just needs testing for MacOS, which I'm not able to do. The AppVeyor and Travis-CI, Linux build passes, but there is an issue on MacOS.

Could you please test the line above on MacOS and modify it, because it appears to have a bunch of random spaces which isn't the case on Linux. It's supposed to just return the number of background jobs/processes.

Other than that, I'd love to hear if you can spot anything I've missed. :slightly_smiling_face:

Snuggle commented 6 years ago

The MacOS issue still occurs, it doesn't seem to be an issue with quotation marks.

It looks like either jobs | wc -l is outputting whitespace, which it shouldn't or there is some issue with how MacOS concatenates the ✦ symbol.

matchai commented 6 years ago

@Snuggle any chance that we could remove jobs from this PR and take care of it separately? The rest of it looks like it's ready to be reviewed.

Snuggle commented 6 years ago

How do you pass args into a mock function? I'm trying to fix my tests since they are based on the current time and the tests can fail if the test takes >1 second to complete (See most recent AppVeyor failure), but I need to pass both the formatting string and set the current time manually.

matchai commented 6 years ago

I'm not sure I totally understand your question.

What is shown in your screenshot should mock the newdate function with an exit code of 0, and should run date --date=@2147483647 $args, replacing $args with its assigned value.

Snuggle commented 6 years ago

I'm trying to fix this bug and I need to call a mock date function that has a set, static time. NOT the current time. But I would need both the date format and the static time as an argument.

mock newdate 0 "date --date=@2147483647"

set time_str "$time_str"(newdate +%I:%M:%S)

I would like the mock function to return the value of date --date=@2147483647 +%I:%M:%S, but I can't figure out how to pass it. At the moment it returns the value of date --date=@2147483647.

matchai commented 6 years ago

Works fine for me 😄

Snuggle commented 6 years ago

Ah, but I need to do newdate "+%I:%M:%S". The time format changes and cannot be in the mock function. It would have to be passed.

matchai commented 6 years ago

In that case you'll probably want to write a function.

Snuggle commented 6 years ago

Gosh, trying to get things working in MacOS through automated testing is a real struggle. I need to fire up a MacOS virtual machine instead of spamming commits.

Snuggle commented 6 years ago

MacOS why?! (╯ರ ~ ರ）╯彡 ┻━┻ Time to rewrite a part of this for MacOS's BSD date command.

matchai commented 6 years ago

Looks ready to merge after those last couple comments 👍 Thank you for taking the time to work on this monster PR! 🎉

Snuggle commented 6 years ago

No problem! It was a pleasure finding a project that I loved and could understand. Open-source can feel really daunting when you're faced with codebases that are monstrously big. Thanks for the thorough reviews and for porting my favourite Zsh prompt theme to Fish! I love the name and aesthetic, too.

(I also need to fix Atom because it's throwing borked whitespace all over the place.)

matchai commented 6 years ago

I'm glad you found this project to be easy to contribute to! I really appreciate your kind words. 😄

matchai / spacefish