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 the bug of spliting strings of configuration values. #146

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Haixing-Hu
Copy link

Some configuration parameters accept the space or comma separated values. For example, the defaultIncludes, defaultExcludes, optionSet, defineSet, undefineSet, and the compileOrder.

The Compiler.getCompiler() function will split those values and add them to the list of compiler arguments.

But the implementation of Compiler.getCompiler() failed to filter the empty sub-strings, and failed to trim the sub-strings, produced by the splitting.

This patch simply uses a regular expression as the separator to filter the empty sub-strings and trim the resulting sub-strings.

@buildhive
Copy link

NAR plugin for Maven » nar-maven-plugin #283 SUCCESS
This pull request looks good
(what's this?)

…iguration, and also change the order of setting defines and undefines.
@buildhive
Copy link

NAR plugin for Maven » nar-maven-plugin #284 SUCCESS
This pull request looks good
(what's this?)

@dscho
Copy link
Member

dscho commented Nov 20, 2014

Thank you for your contribution! I offered some suggestions for improvements, hopefully I was clear. If not, or if you disagree, please do not hesitate to answer my concerns. Again, thanks for contributing to the NAR plugin!

@Haixing-Hu
Copy link
Author

About the unit test.
Since the fields of the Compiler class was set by the IoC, and it provides no public setters nor getters. I have no idea about how to set the fields of a Compiler object in the unit test. Therefore I cannot provide an unit test for that patch.
Could you please give me some suggestions?

@dscho
Copy link
Member

dscho commented Nov 20, 2014

Could you please give me some suggestions?

I thought more about an integration test. Example: https://github.com/maven-nar/nar-maven-plugin/tree/master/src/it/it0003-jni

@dscho
Copy link
Member

dscho commented Dec 1, 2014

@Haixing-Hu still working on it?

@Haixing-Hu
Copy link
Author

sorry that I'm working on a company project and was very busy these days.

I will continue work on the nar-maven-plugin project in my spare time.

在 2014年12月1日,下午5:23,dscho notifications@github.com 写道:

@Haixing-Hu https://github.com/Haixing-Hu still working on it?


Reply to this email directly or view it on GitHub #146 (comment).

@dscho
Copy link
Member

dscho commented Dec 1, 2014

I will continue work on the nar-maven-plugin project in my spare time.

We all do... All the contributors to the nar-maven-plugin, that is. Thank you for your time and efforts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants