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

GH1363: Wixheat pog fix #1360

Merged
merged 1 commit into from Jan 29, 2017
Merged

GH1363: Wixheat pog fix #1360

merged 1 commit into from Jan 29, 2017

Conversation

jnm2
Copy link
Contributor

@jnm2 jnm2 commented Nov 14, 2016

Fixes three issues, each of which completely prevented the parameter from working (at least with WiX 3.10):

  • -pog: x causes Heat to not associate x with -pog and instead sets -pog to an empty string, giving:
    warning HEAT1108 : The command line switch 'pog:' is deprecated. Please use 'pog' instead.
    error HEAT5301 : Invalid project output group: .
    
    Since the colon is deprecated anyway, I changed it simply to -pog x.
  • -pog binaries causes error HEAT5301 : Invalid project output group: binaries., whereas -pog Binaries works. All output group names were capitalized.
  • Satallites is also not recognized by Heat since it is misspelled. I also renamed the enum which is a breaking change to anyone using that value. (I suspect that number is negligible due to the fact that the parameter cannot currently be used without erroring Heat.)

@jnm2
Copy link
Contributor Author

jnm2 commented Nov 14, 2016

@gep13 Does this need an issue? I haven't gotten my head around that yet...

@gep13
Copy link
Member

gep13 commented Nov 14, 2016

@jnm2 yes. Ideally, you would have created the issue first, as per the contribution guidelines', and then implemented the PR.

@gep13
Copy link
Member

gep13 commented Dec 6, 2016

@jnm2 did you create an issue for this PR?

@jnm2
Copy link
Contributor Author

jnm2 commented Dec 6, 2016

@gep13 Yes, #1363

@gep13 gep13 changed the title Wixheat pog fix GH1363: Wixheat pog fix Dec 6, 2016
@jnm2
Copy link
Contributor Author

jnm2 commented Dec 29, 2016

Some time has gone by. Should I rebase?

@gep13
Copy link
Member

gep13 commented Dec 29, 2016

@jnm2 said...
Some time has gone by. Should I rebase?

Apologies, all of us have been snowed under lately, and as a result, we haven't had a chance to look at some of the open PR's that we have. Rest assured, we will be getting to them though.

As a general rule, keeping a PR up to date, and in a state where it can be merged certainly helps with the review process, and it can go a long way to getting your PR merged in sooner, as there isn't that back/forth required between reviewer and contributor. This is not expected to happen though, but if you are happy to rebase, then I would say go for it 👍

@jnm2 jnm2 force-pushed the wixheat-pog-fix branch 2 times, most recently from beb87df to ca0a808 Compare December 29, 2016 12:09
@devlead devlead force-pushed the wixheat-pog-fix branch 4 times, most recently from ecc7a29 to 259bc7c Compare January 14, 2017 19:58
* Fixed misspell 'satallites', broken parameter
* Fixed -pog case-sensitivity, broken parameter
* Fixed -pog colon followed by space, broken parameter
Copy link
Member

@gep13 gep13 left a comment

Choose a reason for hiding this comment

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

LGTM!

@gep13 gep13 merged commit 2e46585 into cake-build:develop Jan 29, 2017
@gep13
Copy link
Member

gep13 commented Jan 29, 2017

@jnm2 thank you very much for your contribution! Your changes have now been merged, and will be included in the next release of Cake.

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.

None yet

3 participants