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

src: condense experimental warning message #45424

Merged
merged 1 commit into from
Nov 13, 2022
Merged

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 11, 2022

Before:

$ node --watch fhqwhgads
(node:26196) ExperimentalWarning: Watch mode is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)

After:

$ ./node --watch fhqwhgads
(node:78033) ExperimentalWarning: Watch mode is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 11, 2022
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Nov 11, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 11, 2022
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member Author

Trott commented Nov 11, 2022

Hmmm...that didn't work...guess the punctuation gets pulled out later on.....

@Trott Trott marked this pull request as draft November 11, 2022 04:38
@Trott
Copy link
Member Author

Trott commented Nov 11, 2022

Ideally, I think, this would be just one sentence (which it is now) and might omit the period if it is being followed by a stack trace but include a period if it isn't.

@Trott
Copy link
Member Author

Trott commented Nov 11, 2022

Leaving the period off when it's a single sentence isn't as weird and noticeable as when it's multiple sentences so I think I'll leave it like that.

@Trott Trott marked this pull request as ready for review November 11, 2022 05:07
@Trott
Copy link
Member Author

Trott commented Nov 11, 2022

Honestly, I wonder if the message would be better as just " is an experimental feature" and that's it. That would probably get these messages under 80 chars for the most part and probably not reduce their effectiveness. "Might change at any time" is kind of weird anyway. It's not going to change at any time. It's going to change when there's a new release and the user updates. I'd think "experimental" is plenty scary enough and we don't need to explicitly warn people that the feature might change.

@Trott Trott changed the title src: add missing punctuation to warning src: condense experimental warning message Nov 11, 2022
@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 11, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 11, 2022
@nodejs-github-bot
Copy link
Collaborator

@aduh95
Copy link
Contributor

aduh95 commented Nov 11, 2022

There are test failures to address.

"Might change at any time" is kind of weird anyway. It's not going to change at any time. It's going to change when there's a new release and the user updates.

I guess it's there to communicate that semver rules don't apply for experimental features.

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2022
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 13, 2022
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 13, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 13, 2022
@nodejs-github-bot nodejs-github-bot merged commit 1255db7 into nodejs:main Nov 13, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 1255db7

@Trott Trott deleted the period branch November 13, 2022 04:39
ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
PR-URL: #45424
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
@ruyadorno ruyadorno mentioned this pull request Nov 24, 2022
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45424
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45424
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
PR-URL: #45424
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
PR-URL: #45424
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants