Skip to content
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

Groups in XCode workspace working. #801

Merged
merged 2 commits into from
Jun 18, 2017
Merged

Conversation

ricka-github
Copy link

I noticed the 'group' directive doesn't do anything for XCode, even though XCode workspaces do support groups.

A nice and simple change to get my feet wet in the codebase.

@starkos
Copy link
Member

starkos commented Jun 7, 2017

Looks reasonable to me. Can you add a unit test for it?

@ricka-github
Copy link
Author

I had to extend the test helper to be able to create a group. It seems to work, but I'm not sure this is the way you want it to be.

@@ -44,7 +44,10 @@
return prj
end


function m.createGroup(wks)
local prj = group ("MyGroup" .. (#wks.groups + 1))
Copy link
Contributor

Choose a reason for hiding this comment

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

just nitpicking I guess... with typically we use 'prj' for projects...
it's a local, so not a big deal, but I would probably rename that local to grp or something.

Copy link
Member

Choose a reason for hiding this comment

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

Technically, we don't need this function at all…you could just use group() directly and have full control over the name. But it doesn't hurt to have it for completeness I suppose?

Either way, the change under test is certainly a good one.

@tvandijck
Copy link
Contributor

Looks good to me... can be merged as is, although I did mark it as 'request changes', it's such a minor thing, that we don't have to hold up the merge for that...

@tvandijck tvandijck merged commit b22de64 into premake:master Jun 18, 2017
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.

3 participants