Skip to content

Conversation

@lefou
Copy link
Member

@lefou lefou commented Jun 2, 2025

Accept the following types (which are all sub-types of os.FilePath):

  • os.SubPath - an sub-path resolved against moduleDir
  • os.RelPath - an relative path resolved against moduleDir
  • os.Path - an absolute path

We also want to accept well-formatted Stringly-typed paths, by the use of the macro-based implicit conversion from os-lib. To make that work, we need to explicitly accept the os.SubPath type. Thus we end up with a os.SubPath | os.FilePath signature, which can be a bit confusing, but gives us compile-time validation instead of potential runtime errors when paths are given as String.

Accept the following tpye:
* `os.SubPath` - an sub-path resolved against `moduleDir`
* `os.RelPath` - an relative path resolved against `moduleDir`
* `os.Path` - an absolute path
* `String` - a sub-path relative to `moduleDir`; the special string `"."` is treated as the `moduleDir`.
@lefou lefou changed the title Impore Task.{Source,Sources} API Improve Task.{Source,Sources} API Jun 2, 2025
@lefou lefou marked this pull request as ready for review June 2, 2025 12:23
@lefou lefou requested review from lihaoyi and lolgab June 2, 2025 13:45
* [[Task.Computed]]s need to be invalidated and re-computed.
*/
inline def Sources(inline values: Result[os.Path]*)(implicit
inline def Sources(inline values: (os.Path | os.SubPath | os.RelPath | String)*)(implicit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This big union type looks pretty out of place. Could we use os.FilePath here? Maybe if we add a few more implicits upstream in OS-Lib to make the conversions from string work?

Copy link
Member Author

@lefou lefou Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Once we use just os.FilePath, we can't profit from String to os.SubPath translation. We either want to support a more generic conversion for String to os.FilePath, which also means we want to handle absolute and relative paths, or find some other way to support conversion.

I committed some compromise acepting os.SubPath | os.FilePath to see if CI likes it.

lefou added 3 commits June 3, 2025 14:43
By using `os.SubPath | os.FilePath`, which is redundant as `os.SubPath` is already a sub-type of `os.FilePath`, we allow implicit converstions from `String` to `os.SubPath`.

The explicit `"."` is a test, too.
@lihaoyi
Copy link
Member

lihaoyi commented Jun 4, 2025

Isn't os.SubPath | os.FilePath just os.FilePath, since os.SubPath is a subclass of it?

@lefou
Copy link
Member Author

lefou commented Jun 4, 2025

@lihaoyi

Isn't os.SubPath | os.FilePath just os.FilePath, since os.SubPath is a subclass of it?

Yeah. Here's my comment from commit 5a28bbb

By using os.SubPath | os.FilePath, which is redundant as os.SubPath is already a sub-type of os.FilePath, we allow implicit converstions from String to os.SubPath.

Also from #5245 (comment)

Once we use just os.FilePath, we can't profit from String to os.SubPath translation. We either want to support a more generic conversion for String to os.FilePath, which also means we want to handle absolute and relative paths, or find some other way to support conversion.

I committed some compromise acepting os.SubPath | os.FilePath to see if CI likes it.

Copy link
Member

@lihaoyi lihaoyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@lefou lefou merged commit b94facc into com-lihaoyi:main Jun 10, 2025
38 checks passed
@lefou lefou deleted the tr-improve-task-source branch June 10, 2025 09:24
@lefou lefou added this to the 1.0.0-RC2 milestone Jun 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants