-
Notifications
You must be signed in to change notification settings - Fork 810
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow creating intermediary groups outside of the project directory #892
Allow creating intermediary groups outside of the project directory #892
Conversation
a0aad0e
to
c5013d5
Compare
@@ -492,7 +492,7 @@ class SourceGeneratorTests: XCTestCase { | |||
let pbxProj = try project.generatePbxProj() | |||
try pbxProj.expectFile(paths: ["Sources", "A", "b.swift"], buildPhase: .sources) | |||
try pbxProj.expectFile(paths: ["Sources", "F", "G", "h.swift"], buildPhase: .sources) | |||
try pbxProj.expectFile(paths: ["../OtherDirectory/C/D", "e.swift"], names: ["D", "e.swift"], buildPhase: .sources) | |||
try pbxProj.expectFile(paths: ["..", "OtherDirectory", "C", "D", "e.swift"], names: [".", "OtherDirectory", "C", "D", "e.swift"], buildPhase: .sources) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there other test cases you can think of for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @segiddins. I'm trying to think of why this wasn't done before. Can you envisage any issues when running this with a different --spec
or --project
path? There aren't enough tests around those use cases.
Could you also add a changelog entry
I'll try to pick this back up next week, thanks for the review! |
c5013d5
to
84f1daf
Compare
@yonaskolb this should be ready now, sorry for the delay! |
@yonaskolb any other changes you'd like me to make? |
// has no valid parent paths | ||
let isRootPath = (isBaseGroup && isOutOfBasePath) || path.parent() == project.basePath | ||
let isRootPath = (isBaseGroup && isOutOfBasePath && isParentOfBasePath) || path.parent() == project.basePath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line can lose the && isParentOfBasePath
.
isOutOfBasePath
captures the intent and isRootPath
is used in the line below. Or am I reading it wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it can. For example, for a project in ., ../foo/bar.txt shouldn't be considered the root path -- the root path should be a strict parent of the current directory
Ping on merging this before the next release? |
Resolves #889
Putting this out as a potential solution -- I'm new to the codebase, so I'm not sure if there's maybe a better way of doing this, or if there's a reason this is unacceptable. Just wanted to try it out, since it makes the projects we're generating via bazel in a scratch directory nicer to edit