-
Notifications
You must be signed in to change notification settings - Fork 95
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 support for displaying artifacts populated via namedSetOfFiles #1356
Conversation
app/invocation/invocation_model.tsx
Outdated
if (event.completed.importantOutput?.length) { | ||
return event.completed.importantOutput; | ||
} | ||
return event.completed.outputGroup?.flatMap((group) => group.fileSets).flatMap((set) => this.files.get(set.id)); |
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.
nit: maybe change to this.files.get(set.id) || []
just to be safe?
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.
Done
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.
Sry I meant like
.flatMap((set) => this.fileSetIDToFilesMap.get(set.id) || [])
(.get()
returns undefined if the entry isn't found in the map)
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.
Will send fix in another PR
app/invocation/invocation_model.tsx
Outdated
@@ -29,7 +29,7 @@ export default class InvocationModel { | |||
brokenTest: build_event_stream.BuildEvent[] = []; | |||
flakyTest: build_event_stream.BuildEvent[] = []; | |||
timeoutTest: build_event_stream.BuildEvent[] = []; | |||
files: build_event_stream.NamedSetOfFiles[] = []; | |||
files: Map<string, build_event_stream.IFile[]> = new Map(); |
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.
nit: consider making this private
so people don't accidentally reference this instead of getFiles
Also consider adding a comment describing what the keys represent, or rename to something like xToFilesMap
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.
Done
This prepares us for the impending flip of the
--legacy_important_outputs
flag in future bazel versions: bazelbuild/bazel#14353Version bump: Patch