-
Notifications
You must be signed in to change notification settings - Fork 212
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
added hasInput funtion to Reader/BuildStep #17
Conversation
/// | ||
/// If [trackAsDependency] is true, then [id] will be marked as a dependency | ||
/// of all [expectedOutputs]. | ||
Future<bool> hasInput(AssetId id, {bool trackAsDependency: true}); |
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.
Is trackAsDependency
something we want to expose to users, or should it be an implementation detail?
I feel like you had a use case for it, but I can't remember what it is right now.
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.
What does it mean if the user passes false for this? Presumable, the caller uses the boolean result of this method to do something that affects their build output. If that boolean were to change without the build step being re-run (because it passed trackAsDependency: false
), won't you end up with stale or inconsistent output?
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.
After some offline discussions I am going to remove this as part of the public api, at least for now, as well as removing the addDependency
function. These combined would allow you to write a builder that cheats a bit in terms of marking its dependencies, which specifically when doing resolution would be useful. We may consider adding it back in the future, but for now it doesn't need to be there.
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.
I will do that in a followup cl though
LGTM |
6081a48
to
76b323d
Compare
This is merged into master now |
cc @nex3 @munificent @kevmoo