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

[Patrick Robb <probb@iol.unh.edu>]

[-- Attachment #1: Type: text/plain, Size: 3473 bytes --]

On Tue, Aug 12, 2025 at 4:31 AM Luca Vizzarro <Luca.Vizzarro@arm.com> wrote:

> 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.
>

Okay, I was thinking this may be the case. I don't think any change is
needed.


>
> >     +    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.
>

That's clear. So, this patch looks good to me, thanks Luca.

[-- Attachment #2: Type: text/html, Size: 4822 bytes --]