[JENKINS-28148] Whitespaces in toolLocations #28
Conversation
|
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
|
I like the first and the last option the best. Which one exactly does your commit now support? |
|
The first one, but I can change it to the last if the API is better |
|
Additionnaly, this kind of API could be even simpler using the Thus I'll change my commit to be this implementation for anyone to test. Open to suggestions :) |
polopi
force-pushed
the
master
branch
3 times, most recently
from
8d6db11 to
f92c396
Compare
Use multiple values for toolLocations parameter instead of only one parameters with all tools, and splitting by whitespace. This allow to include whitespace in tool paths (cf windows) Each tool need to be declared in a distinct option. The API change to : -t toolName=path --toolLocation=path
|
Up |
|
Hi! I'll be on vacation for about a week but I can take a look when I come back. On Saturday, May 16, 2015, PaulP notifications@github.com wrote:
"It is often said that people who respect the importance of footwear are |
|
Hi! Thanks. |
|
Hi! No time so far, tomorrow I'll take a look. On Wednesday, June 17, 2015, Paul Ollivier notifications@github.com wrote:
"It is often said that people who respect the importance of footwear are |
|
So, this is almost a month later and I feel bad. But vacations and general life got in between. I will try to set some time aside for this during this week. |
|
👍 |
|
This looks alright but I think we should bump the major version since this is breaking the existing behaviour. So I would like to release the current version first (1.25). Then move on with this PR. Is that ok with you? |
|
I'm ok with that 👍 |
|
I'll pick this up after 1.26 has reached some users and any problems have been reported back! I think we can re-open this PR so I will not forget about it. |
[JENKINS-28148] Whitespaces in toolLocations
|
I'll do some manual integration tests and then release version 2.0 |
Use multiple values for toolLocations parameter instead of only one
with all tools, and splitting it by whitespace.
It allows to include whitespace in tool paths (cf windows)
Link to ticket: JENKINS-28148
The API of the CLI would now be :
-toolLocation tool1:location -toolLocation tool2:"a location"Another options would be to change the separator of toolLocations. The semi-colon seems to be the best option for this. Leading to :
Note that I tried to find some OptionHandler that would fit best, StringArrayOptionHandler for instance.
-toolLocations tool1:location "tool2:a location"But it does not consider an option with quotes to be only one. Like current behaviour...
There is also the MapOptionHandler that would best fit the need IMO, but also requires to change the CLI API.
-toolLocation tool1=location -toolLocation tool2="a location"I have implemented
the first onethe last one (see below) in this change.Let me know which way do you prefer.