Allow multiple segments in Stringliterals #297
Conversation
|
@lihaoyi could you rerun checks? |
…acros, generalizing macro return type to PathChunk
|
@lihaoyi
After last failed workflow I have changed macro to make StringPathChunk and ArrayPathChunk instead of SubPathChunk, but I dont know why SubPathChunk would fail, expecially only on particular environment and not others |
|
@pawelsadlo will take a look! |
|
@lihaoyi it succeed now, so I dont know if we need to examine it, anyway could do a review? |
|
Will take a look tomorrow |
os/test/src/PathTests.scala
Outdated
| import java.net.URI | ||
| object PathTests extends TestSuite { | ||
| private def nonValidPathSegment(chars: String) = s"[$chars] is not a valid path segment." |
There was a problem hiding this comment.
I think we can do a more user-focused error message here, something like
literal path sequence
<entire-literal>used in OS-Lib must be in a canonical form, please use<literal-without-leading-slashes>instead
There was a problem hiding this comment.
@lihaoyi
I have provided a NonCanonicalLiteral exception for this case
actually there is also an edge case when sanitizing literal results in empty path, for example //, /., . literals , in such cases i throw:
InvalidSegment(
literal,
s"Literal path sequence [$literal] doesn't affect path being formed, please remove it"
)If you have any better suggestions for error messages I can change them.
|
@pawelsadlo looks great, left some comments. If you update the code and update the PR description to have the most up to date state of the PR I think we can merge it |
allowing [..] in literal paths adding literal dedicated compile-errors
moving its tests to os package
…gments-in-string-literals
|
@lihaoyi could you please rerun checks? |
|
@lihaoyi let me know if there are any suggestions left |
|
@pawelsadlo looks great, thanks! I merged it, I assume your bank details haven't changed so I'll pay out the bounty to the same account as last time |
Introduced in com-lihaoyi/os-lib#297, these allow conciseness when working with literal paths/path-segments while limiting it to compile-time literals to avoid potential path traversal issues that may arise from parsing dynamic paths
This reverts commit 4262d5c.
This PR allows ```scala val p: os.Path = "/hello/world" val s: os.SubPath = "hello/world" val r: os.RelPath = "../hello/world" ``` This only allows string-literals that are valid absolute/sub/relative-path respectively; passing in invalid paths (e.g. `val p: os.Path = "hello/world"`) or non-literals (e.g. `val str = "/hello/world"; val s: os.SubPath = str `) is a compile error This builds upon @pawelsadlo's work in #297, mostly using `segmentsFromStringLiteralValidation` unchanged with some light pre/post processing to trim the leading `/` off of absolute `os.Path`s and check for leading `..`s on `os.SubPath`s I'm going to declare bankruptcy on the Expecty issues, as we cannot forever be working around bugs in unrelated libraries. If someone has problems and wants to fix expecty, they can do so, and we don't need to care. If nobody cares enough to fix expecty, we shouldn't care either.
This PR adds support for multi-segment literal strings.
for example
it also parses
..segments from literal asos.upenabling syntax like:non-canonical paths used in literals result in compile-time errors, suggesting usage of canonical paths or removing path segment, eg.
Note:
Its not usable in os-Lib itself, due to the fact that it would lead to macro expansion in the same compilation unit as its definition.
@lihaoyi
there is a little bit of hacking involved:
Pathand particularMacrofiles (also needed to mockacyclic.skippedin case ofscala 3).ViewBoundImplicittrait because macros turn out to be not avaliable as implicit views, this is needed forArrayPathChunkandSeqPathChunkto work.