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

Only omit 5 deprecation warnings per feature by default #1327

Merged
merged 1 commit into from May 22, 2021
Merged

Conversation

nex3
Copy link
Contributor

@nex3 nex3 commented May 22, 2021

@nex3 nex3 requested a review from Awjin May 22, 2021 00:58
void warn(String message,
{FileSpan? span, Trace? trace, bool deprecation = false}) {
if (deprecation) {
var firstParagraph = message.split("\n\n").first;
Copy link
Contributor

Choose a reason for hiding this comment

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

My only concern is that we're now assigning special semantics to \n\n in a warning, which could cause confusion. But, I'm not sure if any other heuristic would make sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could theoretically try to pass in some sort of side-channel "deprecation type" identifier, maybe through Zones, but I think that would be more complicated than would be necessary. Given that we control both this heuristic and the messages that it applies to, I think it's fine.

@nex3 nex3 merged commit 6444f8d into limit-warnings May 22, 2021
@nex3 nex3 deleted the verbose branch May 22, 2021 05:40
nex3 added a commit that referenced this pull request May 22, 2021
/// omitted.
///
/// The [node] flag indicates whether this is running in Node.js mode, in
/// which case it doesn't mention "verbose mode" because the Node API doesn't
Copy link
Contributor

Choose a reason for hiding this comment

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

The Node API takes an option object. Adding support for a verbose option in it would be easy, and would allow getting all deprecation warnings for all users using the npm package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but adding a new feature (even a small one) to the Node.js API is a much more involved process.

Copy link
Contributor

Choose a reason for hiding this comment

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

this one seems easy to add: #1332

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants