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

Add --fatal-deprecation flag #1346

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Add --fatal-deprecation flag #1346

wants to merge 1 commit into from

Conversation

jathak
Copy link
Member

@jathak jathak commented Jun 4, 2021

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 Deprecation and the parameters for Deprecation.message were 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 a removedIn version that should have one?
  • Does the Zone-based approach for setting fatal deprecations make sense here? What about updating withWarnCallback to include an error function to allow Deprecation.warnOrError to 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-deprecation flag), or should Deprecation be exposed as part of the public API?

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.
@jathak jathak requested a review from nex3 June 4, 2021 19:21
@nex3
Copy link
Contributor

nex3 commented Jun 4, 2021

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 Deprecation and the parameters for Deprecation.message were 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 a removedIn version 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 withWarnCallback to include an error function to allow Deprecation.warnOrError to 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-deprecation flag), or should Deprecation be 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 jathak marked this pull request as draft June 9, 2021 17:15
@nex3 nex3 force-pushed the main branch 2 times, most recently from 9cff4ca to 6c59213 Compare July 21, 2023 01:57
@ntkme
Copy link
Contributor

ntkme commented Apr 18, 2024

@jathak I think we can close this as it's already delivered in a different PR?

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