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 Haskell Stack support #546

Merged
merged 26 commits into from
Jan 25, 2020
Merged

feat: add Haskell Stack support #546

merged 26 commits into from
Jan 25, 2020

Conversation

m0nhawk
Copy link
Contributor

@m0nhawk m0nhawk commented Oct 16, 2019

Description

  • Add a Haskell Stack module, when it detects stack.yaml file.

Motivation and Context

Closes #
I have a bunch of Haskell projects and I'm really missing support for Haskell in starship.

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 2019-10-16 at 12 27 15 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.
  • Add support for other Haskell: cabal, .hs files etc.

@matchai matchai requested a review from a team October 28, 2019 13:41
Copy link

@bijancn bijancn left a comment

Choose a reason for hiding this comment

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

Great job of adding Haskell. I have some comments concerning code style

src/modules/haskell.rs Show resolved Hide resolved
src/configs/haskell.rs Show resolved Hide resolved
src/modules/haskell.rs Outdated Show resolved Hide resolved
src/modules/haskell.rs Outdated Show resolved Hide resolved
src/modules/haskell.rs Show resolved Hide resolved
src/modules/haskell.rs Outdated Show resolved Hide resolved
src/modules/haskell.rs Outdated Show resolved Hide resolved
@bijancn
Copy link

bijancn commented Nov 2, 2019

Somehow Ubuntu and Windows Tests are still failing?

@m0nhawk
Copy link
Contributor Author

m0nhawk commented Nov 2, 2019

Somehow Ubuntu and Windows Tests are still failing?

Same Python version issue addressed in this branch: update-python.

@m0nhawk
Copy link
Contributor Author

m0nhawk commented Nov 5, 2019

Rebased on top of master.

@m0nhawk
Copy link
Contributor Author

m0nhawk commented Jan 4, 2020

@bijancn I've rebased to latest master and all the tests are green now.

@bijancn
Copy link

bijancn commented Jan 4, 2020

Sorry @m0nhawk I lost track of this. Looks great to me but I dont think I can merge it due to permissions. @matchai can you help?

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.

The CI workflow will need to be updated to include a haskell-setup action. I'd suggest rebasing over master to make sure the new CI system runs.

Looks good so far 👍
Just a few things need tackling before we can merge it.

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

m0nhawk commented Jan 5, 2020

@matchai I have added mstksg/setup-stack, because this requires stack.yaml, not only the compiler - GHC. Also, I've added few tests.

@m0nhawk
Copy link
Contributor Author

m0nhawk commented Jan 5, 2020

Also, I needed to export $HOME in render_module and render_prompty, because Haskell Stack requires HOME variable. See f2e1004

It would be nice to point how to better make it. 😄

@chipbuster
Copy link
Contributor

@heyrict Would you mind marking the changes you requested as resolved? (Or I can dismiss them for you if you'd like). It looks like this'll be ready to merge pretty soon and those requested changes will block the merge.

Copy link
Member

@heyrict heyrict left a comment

Choose a reason for hiding this comment

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

@chipbuster Oh sorry, I should have approved this.

@chipbuster
Copy link
Contributor

@m0nhawk You can use .env() on the command from within the test--you don't have to modify the file in common, e.g.

  render_module("haskell")
  .use_config("blah")
  .env("HOME", var1)

See the test git_repo_in_home_directory_truncate_to_repo_true on line 443 of the directory tests for an example of how that's used.

We've also been a little skittish about using the user's actual home directory because of a slew of bugs that have come up in testing before (usually our assumptions about the state of user's $HOME is incorrect, which causes the tests to fail even though the module is working), so our preferred method is to make a temporary directory and then use that as the "fake home" for the tests. You can also see that in the function mentioned above.

@m0nhawk
Copy link
Contributor Author

m0nhawk commented Jan 23, 2020

@chipbuster Thank you. I've added env() to test with HOME directory and have a question.

Alternatively, I can make use of STACK_ROOT.

I've also changed the workflow to include the steps to setup Haskell Stack for all OSs, but it's not running (because it's a change from the fork?).

@chipbuster
Copy link
Contributor

@m0nhawk Since the test using HOME is #[ignore]d, I don't think that'll be an issue (I didn't notice that yesterday).

and have a question.

What's that?

...but it's not running

I'll try to figure out what's going on with that.

@m0nhawk
Copy link
Contributor Author

m0nhawk commented Jan 23, 2020

What's that?
Sorry, was not very clear. It is about using STACK_ROOT, but probably it would complicate Github Actions setup.

I'll try to figure out what's going on with that.
I remember that changing workflow definition in fork would result in them not running, but the only thing I can find is: here. Not sure if that's the case.

@chipbuster
Copy link
Contributor

Were the tests running before your latest set of commits? I seem to recall that they were, but I'm not certain.

(If they were, then that suggests that GH got confused by something that you added to the workflow YAML)

@m0nhawk
Copy link
Contributor Author

m0nhawk commented Jan 23, 2020

Yes, it was running right before the workflow change.

@chipbuster
Copy link
Contributor

Hmmm. The file looks like valid YAML, but is there any possibility that it's somehow not valid for GH Actions? Could you try reverting the YAML change and see if the tests run?

Sorry, I'm not particularly a specialist in GH Actions (I'm more familiar with Gitlab CI) and matchai doesn't see anything obviously wrong either, so we might be taking stabs in the dark here.

@m0nhawk
Copy link
Contributor Author

m0nhawk commented Jan 23, 2020

I reverted the changes and now it's running. Not sure what's wrong with it.

@chipbuster
Copy link
Contributor

Well, debugging this will be interesting, as GitHub doesn't seem to have a YAML validator yet (and the only 3rd party one I can find hasn't been updated since Actions was in early beta).

I'll try my best to take some time to play around with this later today, maybe copy the branch into my fork and try opening a (temporary) PR on that. If you figure out what happened before then, please do ping me so we can get this merged!

@m0nhawk
Copy link
Contributor Author

m0nhawk commented Jan 23, 2020

I'll also check this. I've already enabled my fork Github Actions, so I'll check if it runs there.

@m0nhawk
Copy link
Contributor Author

m0nhawk commented Jan 23, 2020

In my fork it fails with:

a step cannot have both the uses and run keys

I'll continue testing with my fork now on. Thanks for help! 😄

@m0nhawk
Copy link
Contributor Author

m0nhawk commented Jan 23, 2020

@chipbuster I managed to make it run on everything, except Windows. Will try that later locally.

@m0nhawk
Copy link
Contributor Author

m0nhawk commented Jan 25, 2020

@chipbuster Windows tests have some strange issues with environment variable, there is only STACK_ROOT and LOCALAPPDATA variables which are required for stack to run, but it doesn't work as expected. I'm ignoring the test for Windows for now.

@chipbuster
Copy link
Contributor

@m0nhawk Does it work on Windows at the moment?

@m0nhawk
Copy link
Contributor Author

m0nhawk commented Jan 25, 2020

Yes, cargo test is running fine on Windows, it's only Github Actions failing.

@chipbuster
Copy link
Contributor

@matchai Are we okay with merging this without functioning Windows CI on the module? It seems that the module and command do work on Windows, but we can't get GH Actions working.

@matchai
Copy link
Member

matchai commented Jan 25, 2020

Yep, that's fine by me.
We'll get around to moving them over to the mocked tests soon enough 👍

@chipbuster
Copy link
Contributor

Alright, let's get this PR merged then!

(I should really get back into Haskell someday)

@chipbuster chipbuster merged commit 6f2c9fb into starship:master Jan 25, 2020
@chipbuster
Copy link
Contributor

@all-contributors Please add @m0nhawk for code, doc, and tests.

@m0nhawk Thanks so much for all your work on this PR, and for being patient with us through a whole bunch of changes in the project structure! We really appreciate it 😄

@allcontributors
Copy link
Contributor

@chipbuster

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

@m0nhawk
Copy link
Contributor Author

m0nhawk commented Jan 25, 2020

@chipbuster Thank you (and other) for help and discussion. I found starship to be really awesome for cross-platform prompt (as a long time user of spaceship, zsh and fish). And learning some Rust was nice. :)

And, of course, started with the missing feature I need.

I will try to fix it for Windows in Github Actions, it looks like some missing environmental variables, except the one I mentioned is needed.

@m0nhawk m0nhawk deleted the haskell-support branch January 25, 2020 18:37
@m0nhawk m0nhawk mentioned this pull request Jan 25, 2020
8 tasks
Szczyp pushed a commit to Szczyp/starship that referenced this pull request Jan 30, 2020
Add a Haskell Stack module when a stack.yaml file is detected
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.

None yet

5 participants