Re: [PATCH 4/6] dts: add artifact module

[Luca Vizzarro <Luca.Vizzarro@arm.com>]

On 12/08/2025 02:25, Patrick Robb wrote:
> On Fri, Jul 25, 2025 at 11:15 AM Luca Vizzarro <luca.vizzarro@arm.com 
> <mailto:luca.vizzarro@arm.com>> wrote:
> 
> 
>     +
>     +@overload
>     +def make_file_path(node: Node, file_name: str, custom_path:
>     PurePath | None = None) -> PurePath: ...
>     +
>     +
>     +@overload
>     +def make_file_path(node: None, file_name: str, custom_path:
>     PurePath | None = None) -> Path: ...
>     +
>     +
>     +def make_file_path(
>     +    node: Node | None, file_name: str, custom_path: PurePath | None
>     = None
> 
> 
> Maybe it makes sense to set a default value of None for node? That way 
> people don't have to pass in None every time they want to make a path on 
> the DTS engine system.

Even though this function is not private, ideally we don't want this to 
be used outside of the artifact module. But we want the Artifact class 
to be used instead. For this reason the node argument is kept without a 
default to match the behaviour of the Artifact constructor.

Could just make this private.

>     +    def open(
>     +        self, file_mode: BinaryMode | TextMode = "rb", buffering:
>     int = -1
>     +    ) -> Union["ArtifactFile", TextIOWrapper]:
>     +        """Open the artifact file.
>     +
>     +        Args:
>     +            file_mode: The mode of file opening.
>     +            buffering: The size of the buffer to use. If -1, the
>     default buffer size is used.
>     +
>     +        Returns:
>     +            An instance of :class:`ArtifactFile`
>     or :class:`TextIOWrapper`.
>     +        """
>     +        if self._fd is not None and not self._fd.closed:
>     +            self._logger.warning(
>     +                f"Artifact {self.path} is already open. Closing the
>     previous file descriptor."
>     +            )
>     +            self._fd.close()
>     +        elif not self._directories_created:
>     +            self.mkdir()
>     +
>     +        # SFTPFile does not support text mode, therefore everything
>     needs to be handled as binary.
>     +        if "t" in file_mode:
>     +            actual_mode = cast(BinaryMode, cast(str,
>     file_mode).replace("t", "") + "b")
> 
> 
> Is it worth logging this event to prevent confusion? (where we change 
> the requested mode to binary mode)
This is an implementation-related internal detail. At a higher level, 
where the user and test writer are interested, the mode is the same as 
specific in `mode`. The only reason why this is happening is because the 
lower level implementation works in binary mode only. Text mode (in 
standard Python too) is just an encoder/decoder wrapper as used here. 
Under the hood all the file operations are done in binary mode.

A user shouldn't be concerned about how it works internally. Quite the 
opposite, introducing logging for this kind of thing will introduce 
confusion. The user should only care about the higher level 
functionality, which is the API.

Please let me know if it's not clear.