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

Add Elm Section #528

Merged
merged 7 commits into from
Oct 13, 2018
Merged

Add Elm Section #528

merged 7 commits into from
Oct 13, 2018

Conversation

ajlende
Copy link
Contributor

@ajlende ajlende commented Oct 11, 2018

Thanks for you pull-request!

Please, make sure you've read CONTRIBUTING.md before submitting this PR.

Description

This adds support for Elm since #253 hasn't been touched in almost a year. I'm making a companion section for getting information for individual projects in #527.

I added some tests as well, but this is my first time doing tests for shell scripts, so let me know if I've done those correctly.

Screenshot

Please, attach a screenshot, if possible.

spaceship-elm-screenshot

@ajlende ajlende changed the title Elm Section Add Elm Section Oct 11, 2018
denysdovhan
denysdovhan previously approved these changes Oct 12, 2018
@salmanulfarzy
Copy link
Member

This is one of the comprehensive feature PR I've seen in this repo, awesome @ajlende 👏

What do you think about this,

Also, is the Elm version changes so often that it needs to be shown in prompt? Would it be useful for users and make the clutter in prompt? Is there any Elm version management systems, like nvm for Node.js?

From #253 (comment)

@ajlende
Copy link
Contributor Author

ajlende commented Oct 13, 2018

Is there any Elm version management systems, like nvm for Node.js?

No, I've been handling needing multiple versions by having differently named binaries. Similar to how Python has python3 vs python, I have elm vs elm18. That was my personal solution to working with both versions. As far as I know, there isn't an official recommended way to handle multiple versions of Elm.

Is the Elm version changes so often that it needs to be shown in prompt?

Historically, a new version of Elm has been released every 6-9 months. However, Elm still hasn't hit the first stable version, so each has had pretty significant backwards-incompatible changes.

Would it be useful for users and make the clutter in prompt?

I originally had this built into #527 (elm_project) because it seemed a little strange to me to show the project's needed Elm version without the installed Elm version. But after seeing #253 and because the configuration was getting out of hand, I decided to break this out into a separate thing. I can merge this back with elm_project if you think it fits better bundled as one.

@salmanulfarzy Thanks for scrutinizing new features to this. To me, that means I'll be able to keep using spaceship-prompt knowing that it's not going to get bloated with useless features.

Copy link
Member

@salmanulfarzy salmanulfarzy left a comment

Choose a reason for hiding this comment

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

Some minor suggestion about code formatting and tests

tests/stubs/elm Outdated Show resolved Hide resolved
tests/elm.test.zsh Outdated Show resolved Hide resolved
@salmanulfarzy
Copy link
Member

Just noted the following from #527 (comment)

is the Elm version changes so often that it needs to be shown in prompt? Would it be useful for users and make the clutter in prompt?

Recently, Elm updated to 0.19.0, so I (and I'd expect many others) still have quite a few 0.18.0 projects and some 0.17.0 (or earlier) projects. Elm doesn't have an equivalent to nvm, so I've personally handled the different versions by just having differently named binaries. This would allow me to see at a glance which elm binary I need to run for a particular project.

This does look like good use case to display version information.

I can merge this back with elm_project if you think it fits better bundled as one.

I think they are better as separate sections. @denysdovhan prefers each section as atomic as possible -- #340 (comment)

@salmanulfarzy salmanulfarzy added the new-feature A PR that implement feature (section, specific behavior, etc). label Oct 13, 2018
ajlende and others added 2 commits October 13, 2018 14:12
Signed-off-by: Salmanul Farzy <salmanulfarzy@gmail.com>
Copy link
Member

@salmanulfarzy salmanulfarzy left a comment

Choose a reason for hiding this comment

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

LGTM. Merging this as previously approved

@salmanulfarzy salmanulfarzy merged commit d53a787 into spaceship-prompt:master Oct 13, 2018
@salmanulfarzy
Copy link
Member

Thank you for contributing @ajlende 🎉

@ajlende ajlende deleted the elm-section branch October 13, 2018 19:50
@salmanulfarzy salmanulfarzy mentioned this pull request Oct 14, 2018
dedene pushed a commit to zenjoy/spaceship-prompt that referenced this pull request Feb 24, 2019
denysdovhan pushed a commit that referenced this pull request May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature A PR that implement feature (section, specific behavior, etc).
Development

Successfully merging this pull request may close these issues.

None yet

3 participants