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

docs(archive,assert,cache,cli,encoding,html,http,net,streams,text): remove unstable Markdown alert #5672

Merged
merged 10 commits into from
Aug 22, 2024

Conversation

iuioiua
Copy link
Contributor

@iuioiua iuioiua commented Aug 11, 2024

No description provided.

Copy link

codecov bot commented Aug 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.21%. Comparing base (afb9a4f) to head (369f302).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5672      +/-   ##
==========================================
- Coverage   96.22%   96.21%   -0.01%     
==========================================
  Files         481      481              
  Lines       38751    38751              
  Branches     5618     5618              
==========================================
- Hits        37289    37286       -3     
- Misses       1420     1423       +3     
  Partials       42       42              

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

@kt3k
Copy link
Member

kt3k commented Aug 14, 2024

This text isn't needed. @experimental suffices.

This doesn't seem aligned with CLI's policy.

@kt3k
Copy link
Member

kt3k commented Aug 14, 2024

I agree with removing the [!WARNING] format for readability in IDE

@iuioiua
Copy link
Contributor Author

iuioiua commented Aug 19, 2024

It's true that that the CLI uses the unstable note. But I still think it's not needed. Hints in the IDE show the @experimental tag, and JSR shows an unstable button (example). This unstable notice doesn't seem to provide any added value.

@iuioiua
Copy link
Contributor Author

iuioiua commented Aug 19, 2024

Furthermore, while the CLI employs this convention, it's not in the Deno Style Guide. I think these reasons provide sufficient justification to deviate.

@kt3k
Copy link
Member

kt3k commented Aug 19, 2024

But I still think it's not needed. Hints in the IDE show the @experimental tag, and JSR shows an unstable button (example).

I disagree with this. JSR rendering of @experimental tag is fine, but @experimental tag in IDE is not visible enough.

The Spinner docs in IDE after this change.

Screenshot 2024-08-19 at 20 31 24

It's at the last line, and it doesn't seem enough alerting. It's likely the users miss this.

Screenshot 2024-08-19 at 20 32 49

@iuioiua
Copy link
Contributor Author

iuioiua commented Aug 20, 2024

That's because @experimental is the last tag. We should make @experimental always come after the initial description.

We can do:

/**
 * <description>
 *
 * @experimental **UNSTABLE**: New API, yet to be vetted.
 */

WDYT?

@kt3k
Copy link
Member

kt3k commented Aug 20, 2024

That looks better to me, but I think it's still important for CLI and STD to have the same policy (formatting) for this to make things less confusing.

@bartlomieju What do you think about the above suggested formatting?

@kt3k
Copy link
Member

kt3k commented Aug 21, 2024

/**
 * <description>
 *
 * @experimental **UNSTABLE**: New API, yet to be vetted.
 */

This now looks a good compromise between consistency with CLI and better rendering in IDE and JSR. Let's do this

@iuioiua iuioiua disabled auto-merge August 21, 2024 05:18
Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

LGTM

@iuioiua iuioiua enabled auto-merge (squash) August 21, 2024 05:22
@iuioiua iuioiua disabled auto-merge August 22, 2024 06:51
@iuioiua iuioiua changed the title docs(archive,cli,html,http,net,streams,text): remove unstable Markdown alert docs(archive,assert,cache,cli,encoding,html,http,net,streams,text): remove unstable Markdown alert Aug 22, 2024
@iuioiua iuioiua enabled auto-merge (squash) August 22, 2024 06:51
@iuioiua iuioiua merged commit dea7d77 into main Aug 22, 2024
17 checks passed
@iuioiua iuioiua deleted the remove-unstable-alert branch August 22, 2024 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants