Uh oh!
There was an error while loading. Please reload this page.
_stream: use full element name in artifact log/list-contents - #2174
_stream: use full element name in artifact log/list-contents#2174c1assik wants to merge 1 commit into
artifact log/list-contents#2174Conversation
| continue | ||
| artifact_logs[obj.name] = obj._get_logs() | ||
| name = ref if isinstance(obj, ArtifactElement) else obj._get_full_name() |
There was a problem hiding this comment.
Perhaps it would be better if ArtifactElement overrode the _get_full_name method or the __full_name variable, instead of having this awkward condition here?
| name=refifisinstance(obj, ArtifactElement) elseobj._get_full_name() | |
| name=obj._get_full_name() |
In ArtifactElement
def_get_full_name(self)
returnself.get_artifact_name()There was a problem hiding this comment.
Added a method _get_full_name() to ArtifactElement.
return self.get_artifact_name(), unconditionally fails with AssertionError: assert key is not None in get_artifact_name() so if the cache key isn't set yet, fall back to super()._get_full_name() (the base Plugin/Element name, which is always safe since it's precomputed earlier in Plugin.init). Once construction finishes and the key is set, it returns the full artifact ref as intended
There was a problem hiding this comment.
I think it makes more sense to set the name in ArtifactElement (you can set it before chaining up to the super class __init__() so that it gets picked up). I feel it would be more elegant than trying to override _get_full_name().
cdbdea6 to
30fd1a2Comparenathanwilliams-ct
commented
Aug 18, 2026
LGTM |
`artifact_log()` and `artifact_list_contents()` were keying their
result dicts by `obj.name`, which is just the bare, project-relative
element name. For elements loaded across a junction, this drops the
owning junction prefix, so e.g. `bst artifact list-contents
element.bst nested.bst:element.bst` would only show one entry, since
both targets resolved to the same key.
Use `obj._get_full_name()` instead, which includes the junction
prefix, falling back to the artifact ref for `ArtifactElement`
targets (i.e. when an artifact is referred to by its ref rather than
its element name) as before.
Also drops a dead `obj.name = {ref: "No artifact cached"}` assignment
in `artifact_list_contents()` that was immediately followed by
`continue` and never used.
Add tests covering `artifact log` and `artifact list-contents` with a
cross-junction element alongside a same-named top-level element.30fd1a2 to
ceb5115Compare
artifact_log()andartifact_list_contents()were keying their result dicts byobj.name, which is just the bare, project-relative element name. For elements loaded across a junction, this drops the owning junction prefix, so e.g.bst artifact list-contents element.bst nested.bst:element.bstwould only show one entry, since both targets resolved to the same key.Use
obj._get_full_name()instead, which includes the junction prefix, falling back to the artifact ref forArtifactElementtargets (i.e. when an artifact is referred to by its ref rather than its element name) as before.Also drops a dead
obj.name = {ref: "No artifact cached"}assignment inartifact_list_contents()that was immediately followed bycontinueand never used.Add tests covering
artifact logandartifact list-contentswith a cross-junction element alongside a same-named top-level element.