Improve cli for tips review #813

Etesam913 · 2022-07-06T21:23:39Z

Summary

Uses the information in #388 to improve the cli for the review_tips_for_task.py and remove_accepted_tip.py scripts.

I tested using rich(cli formatting python package) out on these two scripts first because they are of low importance. Once this is reviewed then more important commands like mephisto wut can potentially be reformatted using rich.

Video of new cli

New Video:

#813 (comment)

Old Video:

new_review_script.mov

🚚 Moved rich console object into a separate file in utils as it can be reused whenever we want to use it in the future.

Etesam913 · 2022-07-06T21:31:36Z

Any areas for improvement?

While this is the visual ui that Somya suggested, I would say that it is a pretty big upgrade compared to what it previously looked like.

mephisto/scripts/local_db/remove_accepted_tip.py

pringshia · 2022-07-07T13:09:22Z

Thanks for starting this off. I think it's a step in the right direction, I have a few suggestions for improvement:

I think we need to be consistent and forward-looking with the color scheme we choose. We should ideally reserve green for success messages. I think using them in the question text here is not a good idea because it will limit us from using green later, and I think red/green is best reserved for success/error messages. Heroku's CLI style guide has some more info on this... they specifically mention reserving yellow/red for warning/errors, I think we should consider adding green too. I think I'd just remove the colors on the prompt altogethere, which leads me to...
Too many colors. Also from Heroku's CLI style guide:

"Be mindful with color. Too many contrasting colors in the same place can quickly begin to compete for the user’s attention. Using just a couple of colors and maybe dim/bolding existing ones can often provide enough contrast."

I see 4 colors being used here (not including white). I think we really just need the error red and maybe a highlight for the user entry stuff (accept/reject). I don't think the rest is necessary.
It would be nice if this script let you see how many total items there were to review, and which one you were on (so progress indicator). Easiest fix is replacing "Current Tip" with something like "Tip 1 of 3"
It would be nice to allow users to skip a task instead of accept/reject
I think the information hierarchy is a little weird. Where it says "Task Names:", I would expect it to say something like "Tips Review"... it's weird to have what is a label as an H1 equivalent. Also this header style will eventually need to likely be consistent across our other scripts, so I think it makes sense to standardize and get it right.
Check out the choices arg for rich's Input feature here, should we be using that for when picking the task name? https://rich.readthedocs.io/en/latest/prompt.html

Etesam913 · 2022-07-07T15:33:02Z

Yeah all those suggestions make sense, here is the updated version

updated_review_script.mov

I wanted to make the second header left-aligned but I couldn't figure out how to do that in the docs.

pringshia · 2022-07-07T15:38:12Z

Nice! It's looking much cleaner now :)

Were you able to try out the choices prop for Input? I didn't see it in the PR but I think you may have not pushed up the code in the video above yet
We need a space after "What is your reason for the bonus?"
Which of the fields here are optional? I feel that the reason and bonus fields should be optional, and this should be communicated through the UI?
Tip Accepted and Bonus Successfully Paid could probably be green

pringshia · 2022-07-07T15:43:52Z

I wanted to make the second header left-aligned but I couldn't figure out how to do that in the docs.

Instead of Markdown, have you tried just Text?

https://rich.readthedocs.io/en/stable/text.html#text-attributes

Etesam913 · 2022-07-07T15:46:01Z

Nice! It's looking much cleaner now :)

1. Were you able to try out the `choices` prop for Input? I didn't see it in the PR but I think you may have not pushed up the code in the video above yet

2. We need a space after "What is your reason for the bonus?"

3. Which of the fields here are optional? I feel that the reason and bonus fields should be optional, and this should be communicated through the UI?

4. Tip Accepted and Bonus Successfully Paid could probably be green

Sorry, I forgot to push my commit, yeah I started using Prompt.ask and it simplifies a lot of the error checking that I was doing, which is good.

I don't think the bonus field can be optional, you either have to pay a bonus or don't pay a bonus. The bonus field does not show if you reject a tip.

I'm not exactly sure how I would make the reason field optional. Should I just add an extra question asking if you want to write a reason? I feel like there might already be too many steps for accepting a tip.

Etesam913 · 2022-07-07T15:47:35Z

I wanted to make the second header left-aligned but I couldn't figure out how to do that in the docs.

Instead of Markdown, have you tried just Text?

https://rich.readthedocs.io/en/stable/text.html#text-attributes

Yeah I could do that. The headers would have to be removed from the markdown to use that, which I guess is fine. The only way to make a list to my knowledge is to use markdown

codecov-commenter · 2022-07-07T15:52:51Z

Codecov Report

Merging #813 (fbc4b3a) into main (71c7b4a) will increase coverage by 0.22%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #813      +/-   ##
==========================================
+ Coverage   64.44%   64.66%   +0.22%     
==========================================
  Files         107      107              
  Lines        9281     9281              
==========================================
+ Hits         5981     6002      +21     
+ Misses       3300     3279      -21

Impacted Files	Coverage Δ
mephisto/data_model/unit.py	`78.14% <0.00%> (+0.54%)`	⬆️
mephisto/abstractions/architects/mock_architect.py	`90.84% <0.00%> (+2.61%)`	⬆️
mephisto/data_model/assignment.py	`61.71% <0.00%> (+3.90%)`	⬆️
...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...fbc4b3a. Read the comment docs.

♻️ Used .format() instead of concatenating strings

Etesam913 · 2022-07-07T16:20:02Z

@JackUrb After adding the rich dependency to my requirements.txt and pyproject.toml I get this error in the Python test /build github action step:

ERROR: In --require-hashes mode, all requirements must have their versions pinned with ==. These do not:
    commonmark<0.10.0,>=0.9.0 from https://files.pythonhosted.org/packages/b1/92/dfd892312d822f36c55366118b95d914e5f16de11044a27cf10a7d71bbbf/commonmark-0.9.1-py2.py3-none-any.whl#sha256=da2f38c92590f83de410ba1a3cbceafbc74fee9def35f9251ba9a971d6d66fd9 (from rich==12.4.4->-r requirements.txt (line 250))

After searching online the solution may be to pin the version of pip that we are upgrading. At least according to this comment:
python-poetry/poetry#3472 (comment)

Workarounds:
* If you are using pip directly then pin it to a version before 20.3
python -m pip install --upgrade pip==20.2.4

JackUrb · 2022-07-07T16:36:35Z

Oy this is tough, I'm thinking the best approach right now would be to drop the rich requirement (considering it's only used in these scripts) for this merge, and for me to fix the poetry + requirements.txt issues more stably (perhaps addressing #768 in the process.

pringshia · 2022-07-08T18:05:55Z

I'm not exactly sure how I would make the reason field optional. Should I just add an extra question asking if you want to write a reason? I feel like there might already be too many steps for accepting a tip.

Agreed that there are already too many steps, I would suggest using smart defaults to streamline/optionalize:

> Would you like to (a)ccept, (r)eject, or (s)kip this tip? (Default: s):
> How much would you like to bonus the tip submitter? (Default: 0.0):

# if not default/0 show:
> What reason would you like to give the worker for this tip? NOTE: This will be shared with the worker.
   (Default: Thank you for submitting a tip!)

Benefits

Since skip is now the default, this way you can just hit Enter, Enter, Enter as you browse through all tasks, and doesn't require prompting for important state-changing actions to take place (e.g. requiring money and budget)
Removes need to ask if you want to give a bonus or not. You just need one question, if the answer is 0, then no bonus is implied. We also set a default value here.
Reason is only shown if bonus is non-zero
Default reason is provided
Emphasize that the reason is shared with the worker. This wasn't clear with the current design and is important to emphasize

…h/Mephisto into improve-cli-for-tips-review

Etesam913 · 2022-07-08T20:59:22Z

Oy this is tough, I'm thinking the best approach right now would be to drop the rich requirement (considering it's only used in these scripts) for this merge, and for me to fix the poetry + requirements.txt issues more stably (perhaps addressing #768 in the process.

Yeah I just made it so that you need the package installed to run the scripts:

Etesam913 · 2022-07-08T21:07:12Z

#813 (comment)

Makes sense. I agree

Here is the video that shows some of the defaults

script_defaults.mov

JackUrb

The rich script changes are really nice. Happy to get this in.

JackUrb · 2022-07-19T15:19:10Z

mephisto/scripts/local_db/remove_accepted_tip.py

+try:
+    from rich import print
+except ImportError:
+    print(
+        "\nYou need to have rich installed to use this script. For example: pip install rich\n"
+    )
+    exit(1)


I'm not sure we need this casing if we're adding rich as a core part of our cli tooling.

JackUrb · 2022-07-19T15:19:19Z

mephisto/scripts/local_db/review_tips_for_task.py

+try:
+    from rich import print
+except ImportError:
+    print(
+        "\nYou need to have rich installed to use this script. For example: pip install rich\n"
+    )
+    exit(1)


JackUrb · 2022-07-19T16:32:28Z

mephisto/scripts/local_db/remove_accepted_tip.py

@@ -96,8 +96,8 @@ def main():

                    removal_response = Prompt.ask(
                        "\nDo you want to remove this tip? (Default: n)",
-                        choices=["y", "n"],
-                        default="n",
+                        choices=[TipsRemovalType.REMOVE, TipsRemovalType.KEEP],


It seems I was wrong about list, but this can still likely be less verbose with [t.value for t in TipsRemovalType]

Etesam913 added 3 commits July 6, 2022 16:41

➕ Added 'rich' dependency for cli formatting

36150ee

🎨 Improved cli for remove_accepted_tip.py

4b92ab8

🚚 Moved rich console object into a separate file in utils as it can be reused whenever we want to use it in the future.

🔥 Removed emojis due to rendering bugs

f3ef7de

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 6, 2022

Etesam913 added the ui/ux label Jul 6, 2022

Etesam913 requested review from pringshia, Rebecca-Qian and s0mya July 6, 2022 21:32

pringshia requested a review from JackUrb July 7, 2022 12:30

pringshia reviewed Jul 7, 2022

View reviewed changes

mephisto/scripts/local_db/remove_accepted_tip.py Show resolved Hide resolved

♻️ Reduced the number of colors & used Prompt.ask

1b7aca9

➕ Added rich to requirements.txt

536648f

♻️ Used .format() instead of concatenating strings

Etesam913 added 2 commits July 7, 2022 13:25

⏪ Reverted requirements.txt and added import error

280e6d8

🔀 Merged with add-tips-examplpe

f4fec0c

Etesam913 added 2 commits July 8, 2022 16:12

Merge branch 'add-tips-example' of https://github.com/facebookresearc…

3662142

…h/Mephisto into improve-cli-for-tips-review

♻️ Added sensible defaults to the cli scripts

41d702b

Etesam913 requested a review from pringshia July 8, 2022 21:09

Etesam913 mentioned this pull request Jul 11, 2022

Adds a tips example #791

Merged

Etesam913 changed the base branch from add-tips-example to main July 14, 2022 18:50

Etesam913 added 2 commits July 14, 2022 15:25

🔀 Merged with main

0af7039

🚛 Moved task_name print into its new function

3528891

Etesam913 changed the base branch from main to version-bump July 15, 2022 16:02

Etesam913 added 2 commits July 15, 2022 12:12

🔥 Replaced code with print_out_task_names()

71e0da9

🔥 Removed unneeded is_number() method

6a3316e

Base automatically changed from version-bump to main July 15, 2022 17:57

📝 Contributing.md updated with new info on packages

e3461e0

🥅 Improved error message when assets dir is missing

8b8f371

JackUrb approved these changes Jul 19, 2022

View reviewed changes

JackUrb mentioned this pull request Jul 19, 2022

Improve cli for feedback review #816

Merged

Etesam913 added 2 commits July 19, 2022 12:12

♻️ Added enums to avoid hardcoding

d4be593

♻️ Used the enum in all applicable places

3330f4c

JackUrb reviewed Jul 19, 2022

View reviewed changes

Etesam913 added 2 commits July 19, 2022 12:56

🐛 Fixed bug with not comparing value of enum

fbc4b3a

💡 Added comments to remove-accepted-tip script

0f6ed38

JackUrb approved these changes Jul 19, 2022

View reviewed changes

Etesam913 merged commit 10de4c1 into main Jul 19, 2022

Etesam913 deleted the improve-cli-for-tips-review branch July 19, 2022 19:30

snyk-bot mentioned this pull request Mar 6, 2023

[Snyk] Upgrade rc-slider from 8.7.1 to 10.1.1 Benjamin-KY/Mephisto#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve cli for tips review #813

Improve cli for tips review #813

Etesam913 commented Jul 6, 2022 •

edited

Loading

Etesam913 commented Jul 6, 2022 •

edited

Loading

pringshia commented Jul 7, 2022 •

edited

Loading

Etesam913 commented Jul 7, 2022

pringshia commented Jul 7, 2022 •

edited

Loading

pringshia commented Jul 7, 2022

Etesam913 commented Jul 7, 2022 •

edited

Loading

Etesam913 commented Jul 7, 2022 •

edited

Loading

codecov-commenter commented Jul 7, 2022 •

edited

Loading

Etesam913 commented Jul 7, 2022

JackUrb commented Jul 7, 2022

pringshia commented Jul 8, 2022 •

edited

Loading

Etesam913 commented Jul 8, 2022 •

edited

Loading

Etesam913 commented Jul 8, 2022 •

edited

Loading

JackUrb left a comment

JackUrb Jul 19, 2022

JackUrb Jul 19, 2022

JackUrb Jul 19, 2022

Improve cli for tips review #813

Improve cli for tips review #813

Conversation

Etesam913 commented Jul 6, 2022 • edited Loading

Summary

Video of new cli

New Video:

Old Video:

Etesam913 commented Jul 6, 2022 • edited Loading

pringshia commented Jul 7, 2022 • edited Loading

Etesam913 commented Jul 7, 2022

pringshia commented Jul 7, 2022 • edited Loading

pringshia commented Jul 7, 2022

Etesam913 commented Jul 7, 2022 • edited Loading

Etesam913 commented Jul 7, 2022 • edited Loading

codecov-commenter commented Jul 7, 2022 • edited Loading

Codecov Report

Etesam913 commented Jul 7, 2022

JackUrb commented Jul 7, 2022

pringshia commented Jul 8, 2022 • edited Loading

Benefits

Etesam913 commented Jul 8, 2022 • edited Loading

Etesam913 commented Jul 8, 2022 • edited Loading

Here is the video that shows some of the defaults

JackUrb left a comment

Choose a reason for hiding this comment

JackUrb Jul 19, 2022

Choose a reason for hiding this comment

JackUrb Jul 19, 2022

Choose a reason for hiding this comment

JackUrb Jul 19, 2022

Choose a reason for hiding this comment

Etesam913 commented Jul 6, 2022 •

edited

Loading

Etesam913 commented Jul 6, 2022 •

edited

Loading

pringshia commented Jul 7, 2022 •

edited

Loading

pringshia commented Jul 7, 2022 •

edited

Loading

Etesam913 commented Jul 7, 2022 •

edited

Loading

Etesam913 commented Jul 7, 2022 •

edited

Loading

codecov-commenter commented Jul 7, 2022 •

edited

Loading

pringshia commented Jul 8, 2022 •

edited

Loading

Etesam913 commented Jul 8, 2022 •

edited

Loading

Etesam913 commented Jul 8, 2022 •

edited

Loading