Skip to content
This repository has been archived by the owner on Feb 3, 2021. It is now read-only.

feat: Add elixir section #190

Merged
merged 8 commits into from
Jun 19, 2019
Merged

feat: Add elixir section #190

merged 8 commits into from
Jun 19, 2019

Conversation

bradcypert
Copy link
Contributor

@bradcypert bradcypert commented May 3, 2019

This PR adds in support for Elixir into SpaceFish. This matches parity of the feature in https://github.com/denysdovhan/spaceship-prompt

Description

Adds in a section for Elixir support into SpaceFish. It checks against iex, exenv and elixir to see if you have an elixir install and only shows in directories that contain mix.ex or other elixir files. It's configureable via the following options:

Variable Default Meaning
SPACEFISH_ELIXIR_SHOW true Show current Elixir version
SPACEFISH_ELIXIR_PREFIX $SPACEFISH_PROMPT_DEFAULT_PREFIX Prefix before the elixir section
SPACEFISH_ELIXIR_SUFFIX $SPACEFISH_PROMPT_DEFAULT_SUFFIX Suffix after the elixir section
SPACEFISH_ELIXIR_SYMBOL 💧· Character to be shown before Elixir version
SPACEFISH_ELIXIR_COLOR magenta Color of Elixir section

Motivation and Context

This helps maintain the mission of absolute parity with Spaceship.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Screenshots (if appropriate):

How Has This Been Tested?

  • I have tested using MacOS
  • I have tested using Linux

This was tested by writing and running unit tests for my code. I wasn't able to find the build instructions, so I'm unsure how to actually build and test this locally, however.

Checklist:

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

Copy link
Owner

@matchai matchai left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @bradcypert! Looks like a solid PR to me. 😄

I just wonder what's going on with the Linux builds on Travis...
The tests look like they should pass, but for whatever reason, it seems we're getting the wrong version in CI.
Is the mock not working? Does Travis already have Elixir installed? 🤔

functions/__sf_section_elixir.fish Outdated Show resolved Hide resolved
functions/__sf_section_elixir.fish Outdated Show resolved Hide resolved
@matchai matchai added the Type: Enhancement New feature or request label May 4, 2019
matchai and others added 2 commits May 4, 2019 14:05
Co-Authored-By: bradcypert <brad.cypert@gmail.com>
Co-Authored-By: bradcypert <brad.cypert@gmail.com>
@bradcypert
Copy link
Contributor Author

@matchai thanks for looking over my PR and showing me a more idiomatic way to handle the search of a string. I'm not 100% sure why tests would be getting a different version on Linux. As far as I know, the Linux image shouldn't have elixir preinstalled on it and it doesn't look like the travis.yml is adding any elixir stuff.

set -l elixir_version

if type -q kiex
set elixir_version $ELIXIR_VERSION

Choose a reason for hiding this comment

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

This is probably the source of the test failures, because you’ve mocked elixir -v, but not set a mock value for $ELIXIR_VERSION or a mock for exenv version-name. I don’t know the specifics for Travis, but a different CI provider I use has a default version of Elixir installed using kiex.


function setup
spacefish_test_setup
mock elixir -v 0 "echo \"Erlang/OTP 21 [erts-10.3.4] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [hipe] [dtrace]

Choose a reason for hiding this comment

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

I recommend stubbing out the value of $ELIXIR_VERSION here or unsetting it entirely.

@bradcypert
Copy link
Contributor Author

@halostatue You were right! I think I just needed to set a value for the Elixir version.

Copy link
Owner

@matchai matchai left a comment

Choose a reason for hiding this comment

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

Happy to see this passing. Looks great!
Thank you for your contribution. 😄

@matchai matchai changed the title Add elixir feat: Add elixir section Jun 19, 2019
@matchai matchai merged commit 9cb48ea into matchai:master Jun 19, 2019
@matchai
Copy link
Owner

matchai commented Jun 19, 2019

@all-contributors please add @halostatue for reviewing

@allcontributors
Copy link
Contributor

@matchai

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

@matchai
Copy link
Owner

matchai commented Jun 19, 2019

@all-contributors please add @halostatue for review

@allcontributors
Copy link
Contributor

@matchai

I've put up a pull request to add @halostatue! 🎉

matchai pushed a commit that referenced this pull request Jun 19, 2019
# [2.6.0](v2.5.0...v2.6.0) (2019-06-19)

### Features

* Add elixir section ([#190](#190)) ([9cb48ea](9cb48ea))
@matchai
Copy link
Owner

matchai commented Jun 19, 2019

🎉 This PR is included in version 2.6.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@bradcypert bradcypert deleted the add_elixir branch June 22, 2019 17:21
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants