On Tue, Aug 12, 2025 at 4:31 AM Luca Vizzarro <luca.vizza...@arm.com> wrote:
> On 12/08/2025 02:25, Patrick Robb wrote: > > On Fri, Jul 25, 2025 at 11:15 AM Luca Vizzarro <luca.vizza...@arm.com > > <mailto:luca.vizza...@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.
