Add `--fatal-deprecation` flag
Partial fix for #633.
This restructures deprecation warnings into a class that gives each deprecation an identifier and structures the warnings themselves primarily based on the instance variables of that Deprecation.
This supports the core use case of making a deprecation fatal from the command line, but I'd like some feedback on the general approach before finishing this PR (mainly adding support to the Dart API and updating any sass-specs with warnings that were tweaked as part of this).
My main questions are:
- Do the deprecations and their identifiers make sense here? I looked for all the deprecation warnings I could find being emitted but it's possible I missed one. I also made the color units breaking change into two deprecations here (requiring deg for hue as one, and requiring % for saturation and lightness as the other) since the warnings were fairly different.
- The properties of
Deprecationand the parameters forDeprecation.messagewere mostly just based on what was necessary to replicate the existing warnings in a structured way. Are there any that don't make sense, or additional properties that should be added? Also, are there any deprecations without aremovedInversion that should have one? - Does the Zone-based approach for setting fatal deprecations make sense here? What about updating
withWarnCallbackto include anerrorfunction to allowDeprecation.warnOrErrorto work? I'm more confident in the former, but I'm less sure about the latter. - Should the Dart API take in deprecation identifiers as strings (like the
--fatal-deprecationflag), or shouldDeprecationbe exposed as part of the public API?
Partial fix for #633.
This restructures deprecation warnings into a class that gives each deprecation an identifier and structures the warnings themselves primarily based on the instance variables of that Deprecation.
This supports the core use case of making a deprecation fatal from the command line, but I'd like some feedback on the general approach before finishing this PR (mainly adding support to the Dart API and updating any sass-specs with warnings that were tweaked as part of this).
My main questions are:
- Do the deprecations and their identifiers make sense here? I looked for all the deprecation warnings I could find being emitted but it's possible I missed one. I also made the color units breaking change into two deprecations here (requiring deg for hue as one, and requiring % for saturation and lightness as the other) since the warnings were fairly different.
The core question is which deprecations are people likely to want to enable or disable in conjunction with one another. The main use-case for separating them out into separate identifiers in the first place is ensuring that users aren't broken by new deprecations, so coalescing multiple older deprecations isn't much of a problem. Given that, I'd make both the color unit deprecations use one identifier.
- The properties of
Deprecationand the parameters forDeprecation.messagewere mostly just based on what was necessary to replicate the existing warnings in a structured way. Are there any that don't make sense, or additional properties that should be added? Also, are there any deprecations without aremovedInversion that should have one?
I think trying to factor out deprecation warnings into their constituent parts, while clever, might be a bit more complex than it needs to be. It would probably end up more flexible if you just use the Deprecations enum as pure identifiers, and still had each warning site construct a full string from scratch.
- Does the Zone-based approach for setting fatal deprecations make sense here? What about updating
withWarnCallbackto include anerrorfunction to allowDeprecation.warnOrErrorto work? I'm more confident in the former, but I'm less sure about the latter.
I'd prefer to avoid the zone-based approach as much as possible—I think it's generally a lot clearer to explicitly pipe things through the APIs.
Here's an idea for an alternate way of handling this: define an InternalLogger class that wraps Logger:
class InternalLogger extends Logger {
/// Returns [logger] if it's an [InternalLogger], and wraps it in one otherwise.
static Logger wrap(Logger logger);
void warnForDeprecation(Deprecation deprecation, String message,
{FileSpan? span, Trace? trace});
}
By default, this just forwards to Logger.warn(..., deprecation: true) but if verbose is false it limits how many of each deprecation warning are printed and if fatalDeprecation isn't empty it throws a SassException instead of emitting a warning. APIs could still take Logger objects, but their internal fields would be InternalLoggers.
That still leaves open the question of how to handle withWarnCallback—you'd probably need to split warn() into an internal function (which takes an optional Deprecation) and an external one (with the same API as currently). I'm not sure why I made the external one take a deprecation parameter in the first place, but you can translate it into a Deprecation.userDefined value.
- Should the Dart API take in deprecation identifiers as strings (like the
--fatal-deprecationflag), or shouldDeprecationbe exposed as part of the public API?
I think if you make Deprecation into a pure enum without also having a bunch of logic of its own, it makes sense to expose it publicly.
@jathak I think we can close this as it's already delivered in a different PR?