-
Notifications
You must be signed in to change notification settings - Fork 356
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
Conversation
void warn(String message, | ||
{FileSpan? span, Trace? trace, bool deprecation = false}) { | ||
if (deprecation) { | ||
var firstParagraph = message.split("\n\n").first; |
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.
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.
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.
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.
/// 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 |
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.
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.
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.
True, but adding a new feature (even a small one) to the Node.js API is a much more involved process.
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.
this one seems easy to add: #1332
Closes #1323
See sass/sass-spec#1657