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

feat: Add an Elm module #879

Merged
merged 14 commits into from Feb 6, 2020
Merged

feat: Add an Elm module #879

merged 14 commits into from Feb 6, 2020

Conversation

m0nhawk
Copy link
Contributor

@m0nhawk m0nhawk commented Jan 25, 2020

Description

  • Add an Elm module if it detects one of elm.json, elm-package.json, elm-stuff directory or *.elm files.

Motivation and Context

Closes #873

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):

Screen Shot 2020-01-25 at 2 04 06 PM

How Has This Been Tested?

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

Checklist:

  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

@m0nhawk
Copy link
Contributor Author

m0nhawk commented Jan 25, 2020

I have tested this on macOS (screenshot), Linux (latest Fedora) and Windows.

Unfortunately, the ghaction-chocolatey doesn't export variables nicely (I believe it's similar I had with Haskell Support in #546) and it fails to run elm on Windows in Github Actions.

Copy link
Member

@andytom andytom left a comment

Choose a reason for hiding this comment

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

Have a few minor corrections for the docs but in general LGTM, I'm not an Elm user so unable to test locally.

docs/config/README.md Outdated Show resolved Hide resolved
docs/config/README.md Outdated Show resolved Hide resolved
@m0nhawk
Copy link
Contributor Author

m0nhawk commented Jan 26, 2020

@andytom Thank you. Somehow I missed few lines to fix.

@andytom andytom changed the title Elm support feat: Add an Elm module Jan 30, 2020
Copy link
Contributor

@jonstodle jonstodle left a comment

Choose a reason for hiding this comment

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

An "elm" entry is missing in src/configs/starship_root.rs.

docs/config/README.md Outdated Show resolved Hide resolved
src/modules/elm.rs Outdated Show resolved Hide resolved
Copy link
Member

@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.

Since this module is a new addition, it would be best if we started using the new mocking functionality introduced in #768, rather than adding a new test dependency.

We've gone ahead and migrated the Node tests to use the mocking mechanism in #867, if you'd like to use it as an example.

@m0nhawk
Copy link
Contributor Author

m0nhawk commented Feb 6, 2020

@matchai I made the necessary changes to implement new tests (at least the tests are passing for this module).

Copy link
Member

@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.

Looks good to me! 👍
Thank you for taking the time to update the tests.

@matchai matchai merged commit 04247d9 into starship:master Feb 6, 2020
@matchai
Copy link
Member

matchai commented Feb 6, 2020

@all-contributors Please add @m0nhawk for code, docs, and test. 🎉

@allcontributors
Copy link
Contributor

@matchai

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

@m0nhawk m0nhawk deleted the elm-support branch February 6, 2020 05:17
matchai pushed a commit that referenced this pull request Feb 6, 2020
matchai pushed a commit that referenced this pull request Feb 6, 2020
dagbrown pushed a commit to dagbrown/starship that referenced this pull request Oct 22, 2021
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.

Add Elm module
4 participants