Feature/refactor path resolve #190
Conversation
pkg/filesystem/path/resolve.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| rs.stack = append(rs.stack, path) | ||
| if remainder != nil { |
There was a problem hiding this comment.
Maybe we can require that Parser.ParseScope() always returns a RelativePath? That way this code can just push it on the stack unconditionally.
There was a problem hiding this comment.
Yeah, in the absence of an error there will always be a remainder. But the two nilable return vaules do not reflect this. As the code is written it is safe to drop the check, if that is desireable (I prefer the belt-and-suspenders).
Or I could refactor the ParseScope function to just return everything with the named output arguments,
and then the error being there is all that matters. How do you handle cases like this?
Something something sum-types
stagnation
force-pushed
the
feature/refactor-path-resolve
branch
2 times, most recently
from
2eab22b
to
05d0bb5
Compare
| @@ -16,74 +16,74 @@ func TestResolve(t *testing.T) { | |||
| ctrl := gomock.NewController(t) | |||
|
|
|||
| t.Run("NullByte", func(t *testing.T) { | |||
| scopeWalker := mock.NewMockScopeWalker(ctrl) | |||
|
|
|||
| _, err := path.NewUNIXParser("hello\x00world") | |||
There was a problem hiding this comment.
Technically speaking this test should be moved into unix_parser_test.go or something, but I have to say I don't care strongly.
There was a problem hiding this comment.
I can do that when we flesh out an alternative implementation :)
stagnation
force-pushed
the
feature/refactor-path-resolve
branch
from
2b53980
to
fb92218
Compare
stagnation
force-pushed
the
feature/refactor-path-resolve
branch
from
fb92218
to
ad3820a
Compare
pkg/filesystem/path/scope_walker.go
Outdated
| // OnRelative() may be called at most once, and both must never be | ||
| // called on the same path. Resolve() will always call into one of |
There was a problem hiding this comment.
What does "and both must never be called on the same path" mean? Isn't this redundant with what's stated before that?
There was a problem hiding this comment.
Yes it is redundant, I wanted to make it super clear but happy to remove it if it is clear enough :)
pkg/filesystem/path/unix_parser.go
Outdated
| } | ||
|
|
||
| // NewUNIXParser creates a Parser for Unix paths that can be used in Resolve. | ||
| func NewUNIXParser(path string) (Parser, error) { |
There was a problem hiding this comment.
Could you maybe reorder the contents of this file to be more logical? For example:
- Utility functions at the top
unixParser,*NewUNIXParser(), and its methodsunixRelativeParserand its methods
pkg/filesystem/path/parser.go
Outdated
|
|
||
| // Parser is used by Resolve to parse paths in the resolution. | ||
| // The Parser implementations will be used by value in several bookkeeping | ||
| // structures, they must be implemented so they can safely be given as values. |
There was a problem hiding this comment.
I think "safely be given as values" is not a very clear way of phrasing it. I would recommend that we state something like this instead:
Implementations of ParseScope() should return a new copy of Parser and leave the current instance unmodified. It is permitted to call ParseScope() multiple times.
The same for RelativeParser.
There was a problem hiding this comment.
Sounds a lot better!
This prepares for platform specific paths.
stagnation
force-pushed
the
feature/refactor-path-resolve
branch
from
ad3820a
to
bab2e10
Compare
|
LGTM! Will merge after you've addressed that gofmt issue. |
Two refactorings to prepare for resolving, parsing and building paths with drive letters on Windows.