Handle imports with same relative path #1262
Conversation
|
I wouldn't mind some feedback on whether tests should be added to this (I assume they should) and what they would look like. I think it'd either need a new fixture or an existing on to be updated but I don't know the preferred path. |
|
Thanks @CraigSiemens, some great changes. Does #1275 also solve your issue? If so perhaps we can merge that first, and you can add your documentation on top. In terms of a test case, adding it to |
|
I tried it out and #1275 does fix the issue too. I'm working on rebasing my branch onto master. I did find some improvements for the |
CraigSiemens
force-pushed
the
handle-imports-with-same-relative-path
branch
from
b877e65 to
8c894a8
Compare
CraigSiemens
force-pushed
the
handle-imports-with-same-relative-path
branch
from
8c894a8 to
220b388
Compare
| public init(path: Path, basePath: Path) throws { | ||
| var cachedSpecFiles: [Path: SpecFile] = [:] | ||
| let spec = try SpecFile(filePath: path, basePath: basePath, cachedSpecFiles: &cachedSpecFiles) | ||
| let spec = try SpecFile(path: path) |
There was a problem hiding this comment.
This is revering a change from #1275. Internally the init creates the cachedSpecFiles so callers don't have to make it.
| if let specFile = cachedSpecFiles[filePath] { | ||
| return specFile | ||
| } else { | ||
| return try SpecFile(include: include, basePath: basePath, relativePath: relativePath, cachedSpecFiles: &cachedSpecFiles, variables: variables) | ||
| } |
There was a problem hiding this comment.
filePath, which is for the current spec, is not the correct key to use when looking for the specFile of an include that has it's own path.
|
@yonaskolb I believe I'm done rebasing/fixing so it's free for a re-review when you have a moment. I've added in tests to I also made a couple minor changes to the usage of |
|
Thanks @CraigSiemens, I can hopefully get to this soon. @ma-oli would you mind casting an eye over this too, as you were recently in this area. |
|
I’m off right now until Monday but I’m happy to have a look when I’m back. |
|
Hey @ma-oli, have you had a chance to take a look at it yet? |
|
Hey; sorry about the delay. I'm looking at it now and I'm seeing some issues. I'll keep you posted. |
|
This issue has also come in #1299 |
| try self.init(filePath: path, basePath: path.parent(), cachedSpecFiles: &cachedSpecFiles, variables: variables) | ||
| public init(path: Path, basePath: Path? = nil, cachedSpecFiles: inout [Path: SpecFile], variables: [String: String] = [:]) throws { | ||
| let basePath = basePath ?? path.parent() | ||
| let filePath = try path.relativePath(from: basePath) |
There was a problem hiding this comment.
Using a relative path ( "-r ." for instance) as a project root results in this line throwing an unmatchedAbsolutePath error. We might want to use basePath?.absolute(), either here or in SpecLoader.
There was a problem hiding this comment.
That's interesting, I'll try and take a look at that.
|
@ma-oli @yonaskolb I've added the fix for handling relative project paths. |
|
@yonaskolb Any chance this could get merged since it's got an approval? |
Yep, could you please add a changelog entry. Also does this fix #1299? |
…th-same-relative-path
|
Yeah, it should fix that issue. I've merged in master and updated the changelog. |
There was a problem hiding this comment.
Fantastic, thanks for all your work on this @CraigSiemens 🙏
filePathcauses the second to be ignored. #1261Changes
filePathwhich can be relative to thebasePathand not unique.