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

Improve cli for mephisto wut #820

Merged
merged 9 commits into from
Jul 19, 2022
Merged

Improve cli for mephisto wut #820

merged 9 commits into from
Jul 19, 2022

Conversation

Etesam913
Copy link
Contributor

@Etesam913 Etesam913 commented Jul 12, 2022

Summary

Improved the cli of the mephisto wut command and general cli commands like mephisto.

It should be easier to see options for commands and errors should be more clear.

Tables should be much easier to read.

This is done with the rich-click and rich packages.

The rich-click package gives rich formatting to cli commands.

import rich_click as click

this replaces the default click object to get basic rich formatting.

The rich package is still needed for printing things to the console, such as tables.

Video

mephisto-cli-improvement.mov

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 12, 2022
@Etesam913 Etesam913 requested review from pringshia and JackUrb and removed request for pringshia July 12, 2022 16:30
@JackUrb
Copy link
Contributor

JackUrb commented Jul 12, 2022

Ok I really need to fix our deps to get rich required correctly - this is awesome!

@codecov-commenter
Copy link

codecov-commenter commented Jul 12, 2022

Codecov Report

Merging #820 (fde8b3c) into main (71c7b4a) will increase coverage by 0.16%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #820      +/-   ##
==========================================
+ Coverage   64.44%   64.60%   +0.16%     
==========================================
  Files         107      107              
  Lines        9281     9281              
==========================================
+ Hits         5981     5996      +15     
+ Misses       3300     3285      -15     
Impacted Files Coverage Δ
mephisto/abstractions/architects/mock_architect.py 90.84% <0.00%> (+2.61%) ⬆️
...tractions/architects/channels/websocket_channel.py 76.56% <0.00%> (+8.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71c7b4a...fde8b3c. Read the comment docs.

@pringshia
Copy link
Contributor

🤤

@Etesam913 Etesam913 changed the title [WIP] Improve cli for mephisto wut Improve cli for mephisto wut Jul 18, 2022
Copy link
Contributor

@JackUrb JackUrb left a comment

Choose a reason for hiding this comment

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

This is amazing. Excited to have the better mephisto wut experience! Be sure to sync up the rich versions across these PRs

)
return
elif abstraction == "provider":
# TODO: Use markdown list here
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have a plan for executing this TODO, or abandoning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I already executed it, the print_out_valid_options method uses the markdown list.

I forgot to remove the TODO though.

@Etesam913 Etesam913 merged commit 7bfb63e into main Jul 19, 2022
@Etesam913 Etesam913 deleted the improve-cli-for-mephisto-wut branch July 19, 2022 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants