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

Fix buildConfigConstants parameter #723

Conversation

WonderCsabo
Copy link
Contributor

Fixes #722.

This should not be a read-only parameter, because it is configured by the user.
@william-ferguson-au
Copy link
Contributor

How was the test (NB not an example) passing?

@WonderCsabo
Copy link
Contributor Author

@william-ferguson-au sorry i am not sure what you mean. I ran mvn install, and everything passed.

BTW, the CI should be fixed. :)

@william-ferguson-au
Copy link
Contributor

william-ferguson-au commented Apr 22, 2016

This file 7882632#diff-48446e85d4a0808f1f3e2975d47c60b5 is a test POM. It is not an example.

I want to know why the tests didn't fail BEFORE you made the change.
Is that test POM never used?
Do the tests not check the output in BuildConfig?
Do the tests not check the correct thing?

@WonderCsabo
Copy link
Contributor Author

Sorry for the confusion. I will rephrase my second commit message.

Apparently, Maven accepted the inner element names that way, so it was still working before!

@WonderCsabo WonderCsabo force-pushed the 722_fixBuildConfigConstants branch from 7882632 to 4aaa281 Compare April 22, 2016 10:51
@william-ferguson-au william-ferguson-au merged commit 2d7ab13 into simpligility:master Apr 22, 2016
@WonderCsabo WonderCsabo deleted the 722_fixBuildConfigConstants branch April 22, 2016 14:51
@mosabua
Copy link
Member

mosabua commented Apr 22, 2016

Maven collections only work on the outter tag. The inner tag just has to be valid. The tag name is arbitrary. So in this example it could have been

<buildConfigConstants>
  <foo>
    <name>Title</name>
    <value>${project.name}</value>
    <type>String</type>
  </foo>

I am still okay with the merge though so thanks.. we need to probably update the changelog..

@william-ferguson-au
Copy link
Contributor

Changelog already updated.

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