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

use ';' as default separator for vs #1160

Merged
merged 1 commit into from
Oct 18, 2018
Merged

Conversation

WorldofBay
Copy link
Contributor

This is a possible minimal workaround for #1157 .

Gmake2 keeps using " " as default list separator and VS201x gets the ";" it needs as default. It is still changeable in propertydefinitions. The field separator still does two different things at once but now it is at least possible to use list properties in VS201x projects.

Copy link
Member

@samsinsane samsinsane left a comment

Choose a reason for hiding this comment

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

This seems fine to me, however, you'll need to fix the unit tests. Can you also add a unit test for the rule.expandString separator case?

@WorldofBay
Copy link
Contributor Author

I changed the test and added one for a custom separator.

I have no idea what to capture in the gmake2 version though. Maybe someone else can help?

@tdesveauxPKFX
Copy link
Contributor

I guess you can test it like this

--- a/modules/gmake2/tests/test_gmake2_file_rules.lua
+++ b/modules/gmake2/tests/test_gmake2_file_rules.lua
@@ -37,8 +37,14 @@
                        switch = "-p2"
                }

+               propertydefinition {
+                       name = "TestPropertySeparator",
+                       kind = "list",
+                       separator = ","
+               }
+
                buildmessage 'Rule-ing %{file.name}'
-               buildcommands 'dorule %{TestProperty} %{TestProperty2} "%{file.path}"'
+               buildcommands 'dorule %{TestProperty} %{TestProperty2} %{TestPropertySeparator} "%{file.path}"'
                buildoutputs { "%{file.basename}.obj" }

                wks = test.createWorkspace()
@@ -284,3 +290,26 @@ test2.obj: test2.rule
        $(SILENT) dorule -p -p2 "test2.rule"
                ]]
        end
+
+
+       function suite.propertydefinitionSeparator()
+
+               rules { "TestRule" }
+
+               files { "test.rule", "test2.rule" }
+
+               testRuleVars { TestPropertySeparator = { "testValue1", "testValue2" } }
+
+               prepare()
+               test.capture [[
+# File Rules
+# #############################################
+
+test.obj: test.rule
+       @echo Rule-ing test.rule
+       $(SILENT) dorule   testValue1,testValue2 "test.rule"
+test2.obj: test2.rule
+       @echo Rule-ing test2.rule
+       $(SILENT) dorule   testValue1,testValue2 "test2.rule"
+               ]]
+       end

@WorldofBay
Copy link
Contributor Author

WorldofBay commented Oct 8, 2018

i added your test and one for no separator

for some reason the actual output without separator adds an \

for now i add it to the test though i don't know if that is right

@WorldofBay
Copy link
Contributor Author

updated my branch now, all tests pass again

@samsinsane
Copy link
Member

The changes look good, are you able to squash the commits? Once squashed it should be good to merge, thanks!

@WorldofBay
Copy link
Contributor Author

first time for me, so i'm unsure what i should do with the merged commits.
squash, keep, drop?

btw do i have to update the branch before (7 commits behind)?

@samsinsane
Copy link
Member

You don't need to update the branch, you can rebase it if you'd like.

As for the merged commits, dropping them seems like the right thing to do here. I think squashing might pull in the changes from master, which wouldn't be weird, keeping the commit isn't the end of the world but it's kind of a useless merge since you're several commits behind master now. That's my thought process behind this, if you feel differently, I'm happy to discuss. 😄

@WorldofBay
Copy link
Contributor Author

Squashed my commits and dropped the others.

@samsinsane samsinsane merged commit 9e66ec9 into premake:master Oct 18, 2018
@samsinsane
Copy link
Member

Thanks!

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