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

fix: replace halo with yaspin #19

Merged

Conversation

johnson2427
Copy link
Contributor

@johnson2427 johnson2427 commented May 30, 2024

Description

Fixes #18
This is a change to fief-python

Halo is used to produce the spinner in the cli and IPython. This causes an error, and since halo is no longer maintained, we have replaced halo with yaspin

Copy link
Member

@frankie567 frankie567 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, looks good 🙏 The only change needed is to move the dependency in the right group (see comment above).

pyproject.toml Outdated
@@ -46,6 +46,7 @@ dependencies = [
"pytest-mock",
"respx",
"ruff",
"yaspin",
Copy link
Member

Choose a reason for hiding this comment

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

The dependency should be in the cli group and replace halo (line 116).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it, should have searched for halo in the pyproject before pushing. Thank you!

@johnson2427
Copy link
Contributor Author

Had an error in testing, I have all tests passing locally, hoping the last push gets us there

Copy link

codecov bot commented Jun 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (b25db81) to head (d7905fc).

Current head d7905fc differs from pull request most recent head a8ae24b

Please upload reports for the commit a8ae24b to get more accurate results.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #19   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines          689       670   -19     
=========================================
- Hits           689       670   -19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@frankie567
Copy link
Member

Can you give me rights to edit the PR (Allow edits from maintainer on the right)? I've a fix for failing tests.

@johnson2427 johnson2427 force-pushed the johnson2427/fix/replace-halo-with-yaspin branch from d7905fc to a8ae24b Compare June 3, 2024 13:46
@fubuloubu
Copy link

Can you give me rights to edit the PR (Allow edits from maintainer on the right)? I've a fix for failing tests.

We looked for a solid 20 minutes and can't figure out where github moved this option 😅

Maybe it's some configuration we need to update, but github docs no help there either

@johnson2427
Copy link
Contributor Author

This is what I have over there
image

@frankie567
Copy link
Member

Ok! Editing PR from forks is always a nightmare 😅 I'll merge it as it and apply my changes on the main branch.

Thank you for the work on this 🙏

@frankie567 frankie567 merged commit f45d13f into fief-dev:main Jun 4, 2024
@fubuloubu fubuloubu deleted the johnson2427/fix/replace-halo-with-yaspin branch June 4, 2024 13:51
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.

Unhandled error printout with cli client
3 participants