Repository navigation
Conversation
jathak
left a comment
jathak
left a comment
There was a problem hiding this comment.
Good catch with this! --fatal-deprecation <version> should definitely not make deprecations that were obsolete as of <version> fatal.
However, I actually think it makes more sense to exclude all deprecations that are obsolete in the currently running version from --fatal-deprecation, even if they weren't yet obsolete in the <version> passed to the flag.
If for instance, --fatal-deprecation 1.91.0 was added to a user's config when 1.91.0 was the current version, it makes sense for the flag to continue to work without warnings in 1.92.0 when mixed-decls and type-function are obsoleted without the user having to bump the fatal version to 1.92.0, which would additionally make with-private fatal.
| /// Returns the set of all deprecations done in or before [version]. | ||
| static Set<Deprecation> forVersion(Version version) { | ||
| var range = VersionRange(max: version, includeMax: true); | ||
| static Set<Deprecation> forVersion(Version version, [Version? current]) { |
There was a problem hiding this comment.
A comparison to the current version is not necessary here. This should just exclude any deprecation where obsoleteIn is non-null
|
I admit I had to read your last paragraph 5 times before it finally sank in 😆 You are perfectly right. The tests are somewhat lacking, because I don't know how stub the obsoletion data or sass version numbers. (Edit: Sorry for all the |
| final version = Version.parse('1.79.0'); | ||
| final deprecations = Deprecation.forVersion(version); | ||
|
|
||
| expect( |
There was a problem hiding this comment.
Just check that a couple of the currently obsolete deprecations are excluded here. We use deprecations.yaml in the language repo as a single-source of truth for the current list of deprecations and don't want to have to manually update the list anywhere else.
There was a problem hiding this comment.
I understand. I added two regression tests now (so that I don't break the current functionality) and one test for the intended fix (excluding obsolete deprecations).
Using `--fatal-deprecation 1.92.0` would emit: "Warning: mixed-decls deprecation is obsolete, so does not need to be made fatal". This commit avoids these warnings by excluding obsolete deprecations. See also #2647
| @@ -1,3 +1,7 @@ | |||
| ## 16.0.2 | |||
|
|
|||
| * No user-visible changes. | |||
There was a problem hiding this comment.
I wasn't sure about this (whether this is the Dart API or the JS API or both and if they were affected or not).
There was a problem hiding this comment.
Yeah, Deprecation.forVersion is only exposed in the Dart API. I've added a changelog entry for this to the main Dart Sass changelog
jathak
left a comment
jathak
left a comment
There was a problem hiding this comment.
Thanks for this! Sorry for the delay in reviewing; missed it last week and then had jury duty this week.
2 participants
Using
--fatal-deprecation 1.93.3causes unrelated warnings:I think it is because
Depdecation.forVersiondoes not filter out deprecations that have been marked asobsoleteIn.This is my attempt to solve it. I hope I don't have to sign the Google Individual Contributor License Agreement, because I don't use a Google Account (I see no way to sign the agreement without an account, though I'm fine with agreeing to its contents, although my code is just a single conditional and the obvious related test). Also, the newly added test passes, and the grinder linted my code (as far as I can tell), but I could not run the -x node tests for some reason.
If this is unacceptable feel free to close this PR. It was a nice exercise learning something about Dart! Thank you for your time.
PS: This project is so well maintained and I'm grateful to @nex3 who kept in in shape for years and put so much effort in documentation and figuring out a path to go forward with the SASS syntax as CSS evolves more and more. Thank you!
Related: #2647