Various refactors in preparation for adding error reporting #1779
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This allows the solver to just throw an error on failure, rather than wrapping it in an object to eventually get rethrown.
natebosch
reviewed
Jan 11, 2018
| @@ -0,0 +1,47 @@ | |||
| // Copyright (c) 2017, the Dart project authors. Please see the AUTHORS file | |||
munificent
approved these changes
Jan 18, 2018
lib/src/solver/failure.dart
Outdated
| MissingFeatureException(String package, this.version, this.feature, | ||
| Iterable<Dependency> dependencies) | ||
| : super(package, dependencies); | ||
| String toString() => "Tough luck, Chuck!"; |
There was a problem hiding this comment.
Add a TODO to put something real here?
| } else { | ||
| return "${terms.first.package.toTerseString()} is incompatible with " | ||
| "the current SDK"; | ||
| } |
There was a problem hiding this comment.
Might be useful to show the SDK version number here, no?
|
|
||
| String toString() { | ||
| if (cause == IncompatibilityCause.dependency) { |
There was a problem hiding this comment.
What do you think of moving most of this logic into IncompatibilityCause? Something like:
class Incompability {
String toString() => cause.describe(terms);
}
abstract class IncompatibilityCause {
String describe(List<Term> terms);
}
class SdkCause implements IncompatibilityCause {
String describe(List<Term> terms) {
// ...
}
}?
There was a problem hiding this comment.
It always weirds me out a bit to have a method like Incompatibility.describe() that's only ever supposed to be invoked by one specific caller... it feels like kind of the wrong separation of concerns.
nex3
force-pushed
the
feature.solver.error-prep
branch
from
f8eeafc to
38f8976
Compare
This will help us explain version lock to users
Previously, we stopped conflict resolution as soon as we knew that no solution could be found. Now we keep deriving new incompatibilities until we reach one that says the root package can't be selected. This allows us to provide a clearer line of reasoning about why version solving failed to the end user.
The root package is now displayed without a version number or constraint, and PackageRef now properly omits the source name for hosted packages and not for other sources instead of vice versa.
This ensures that constraints constructed by the version solver (rather than authored by humans) that look like ^ constraints are displayed tersely.
Unlike the previous SolveFailure architecture, which had separate subclasses for each type of failure, this includes a single IncompatibilityCause that transitively describes the full reason that the root package couldn't be selected successfully.
Non-derived incompatibilities now indicate their causes, and terms with any constraints no longer list their constraints explicitly.
nex3
force-pushed
the
feature.solver.error-prep
branch
from
38f8976 to
3cde899
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sending these out on their own to avoid dumping another giant pull
request.
See #912