-
Notifications
You must be signed in to change notification settings - Fork 630
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
docs(cli): improve spinner.message document #4785
Conversation
eff5d38
to
21c9b8e
Compare
Changing message seems already supported? See #4079 |
21c9b8e
to
7a62478
Compare
That's correct. The feature is already available. I've updated the commit message and PR title to better reflect the change. Reason - I did not realize you could change So, I figured it would make more sense to use a formal property here so it's both consistent with |
7a62478
to
abdd318
Compare
This sounds rather doc issue, or UI issue of JSR. How about updating the API doc of |
To me this is more about consistency and readability of the class. The act of creating a property screams this is meant to be public. Where, a field could just be something you accidentally forgot to make private explicitly. Also, fields tend to be located at the top which then makes interlacing larger comments between fields a bit unwieldy imo. Another thing that is potentially an issue is usage of Anyways, I looked around and see there is some precedence for making fields public already. I'm happy to comment the field versus making is a property. If any of my thoughts resonate let me know and I'm happy to adjust. An interesting note here is that LSP's seem to show both the same (property in vscode & field in neovim), but |
abdd318
to
ccdc5aa
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4785 +/- ##
==========================================
- Coverage 91.97% 91.97% -0.01%
==========================================
Files 487 487
Lines 41539 41542 +3
Branches 5365 5365
==========================================
+ Hits 38207 38208 +1
- Misses 3274 3276 +2
Partials 58 58 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice docs! LGTM
cli/spinner.ts
Outdated
* } | ||
* | ||
* spinner.stop(); | ||
* spinner.message = "Done!"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just realized this should be console.log
since setting the message after stop
won't do anything. I'll fix!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM again. Thanks for your contribution!
This makes the
Spinner
in@std/cli
expose amessage
property to be consistent with the existingcolor
property. It also adds doc comments and an examples for bothmessage
andcolor
NOTE: I noticed another issue while making this change. The
Color
type also includesAnsi
which is defined as a string. However, the actual code in the setcolor
property will never let you set this to anything other then a validCOLORS
value.