<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">On Tue, Aug 12, 2025 at 4:31 AM Luca Vizzarro <<a href="mailto:Luca.Vizzarro@arm.com">Luca.Vizzarro@arm.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 12/08/2025 02:25, Patrick Robb wrote:<br>
> On Fri, Jul 25, 2025 at 11:15 AM Luca Vizzarro <<a href="mailto:luca.vizzarro@arm.com" target="_blank">luca.vizzarro@arm.com</a> <br>
> <mailto:<a href="mailto:luca.vizzarro@arm.com" target="_blank">luca.vizzarro@arm.com</a>>> wrote:<br>
> <br>
> <br>
> +<br>
> +@overload<br>
> +def make_file_path(node: Node, file_name: str, custom_path:<br>
> PurePath | None = None) -> PurePath: ...<br>
> +<br>
> +<br>
> +@overload<br>
> +def make_file_path(node: None, file_name: str, custom_path:<br>
> PurePath | None = None) -> Path: ...<br>
> +<br>
> +<br>
> +def make_file_path(<br>
> + node: Node | None, file_name: str, custom_path: PurePath | None<br>
> = None<br>
> <br>
> <br>
> Maybe it makes sense to set a default value of None for node? That way <br>
> people don't have to pass in None every time they want to make a path on <br>
> the DTS engine system.<br>
<br>
Even though this function is not private, ideally we don't want this to <br>
be used outside of the artifact module. But we want the Artifact class <br>
to be used instead. For this reason the node argument is kept without a <br>
default to match the behaviour of the Artifact constructor.<br>
<br>
Could just make this private.<br></blockquote><div><br></div><div>Okay, I was thinking this may be the case. I don't think any change is needed.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + def open(<br>
> + self, file_mode: BinaryMode | TextMode = "rb", buffering:<br>
> int = -1<br>
> + ) -> Union["ArtifactFile", TextIOWrapper]:<br>
> + """Open the artifact file.<br>
> +<br>
> + Args:<br>
> + file_mode: The mode of file opening.<br>
> + buffering: The size of the buffer to use. If -1, the<br>
> default buffer size is used.<br>
> +<br>
> + Returns:<br>
> + An instance of :class:`ArtifactFile`<br>
> or :class:`TextIOWrapper`.<br>
> + """<br>
> + if self._fd is not None and not self._fd.closed:<br>
> + self._logger.warning(<br>
> + f"Artifact {self.path} is already open. Closing the<br>
> previous file descriptor."<br>
> + )<br>
> + self._fd.close()<br>
> + elif not self._directories_created:<br>
> + self.mkdir()<br>
> +<br>
> + # SFTPFile does not support text mode, therefore everything<br>
> needs to be handled as binary.<br>
> + if "t" in file_mode:<br>
> + actual_mode = cast(BinaryMode, cast(str,<br>
> file_mode).replace("t", "") + "b")<br>
> <br>
> <br>
> Is it worth logging this event to prevent confusion? (where we change <br>
> the requested mode to binary mode)<br>
This is an implementation-related internal detail. At a higher level, <br>
where the user and test writer are interested, the mode is the same as <br>
specific in `mode`. The only reason why this is happening is because the <br>
lower level implementation works in binary mode only. Text mode (in <br>
standard Python too) is just an encoder/decoder wrapper as used here. <br>
Under the hood all the file operations are done in binary mode.<br>
<br>
A user shouldn't be concerned about how it works internally. Quite the <br>
opposite, introducing logging for this kind of thing will introduce <br>
confusion. The user should only care about the higher level <br>
functionality, which is the API.<br>
<br>
Please let me know if it's not clear.<br></blockquote><div><br></div><div>That's clear. So, this patch looks good to me, thanks Luca. </div></div></div>