Now quoting all #583
Now quoting all #583
Conversation
|
Needs some sample Grinders run against this PR |
|
I would like to understand the need for this PR. IMHO, adding quotes where possible doesn't seem necessary. Please remember to follow the KISS principle. |
|
The non quoted strings are simply wrong. From time to time, someone use a string with space (does not meter if it is "vendor id" or "some path". All have spaces allowed. And will got a cryptic,. unrelated error later, which is wasting unnecessary time. |
Sure thing, will do. |
That is usntbale, but it is correct failure. For sake of sanity, https://ci.adoptium.net/view/Test_grinder/job/Grinder/10678/ passed @smlambert @sophia-guo @llxia pls reconsider |
|
Ping please? |
1 similar comment
|
Ping please? |
|
I am not enamoured with this PR. There was no report of anyone actually encountering difficulties without it. We tend to not create fixes for problems that are not reported. This change has some potential for side-effects (especially given the quotes are around the entire variable=value part, and there was also no Grinders run against the PR branch, the ones listed were against a different branch. |
|
If I would not hit it, I would not be fixing it. On every other project here, the quoting is mandatory. Why not here? The believe of project maintainers that users do not use spaces (..in filesystem.., not speaking about others) is always misplaced. The grinder run is correct. I did not now better way then to make custom branch in aqa-tests, where was only single change in get.sh to pull this-chnaged-pr. https://github.com/judovana/aqa-tests/commits/myTkg/ -> judovana/aqa-tests@537126f Sure, feel free to close. But it will just bite later, and waste another developer's time, because the error caused by ill parsed space is appearing later, and is hard to read. It does not metter if you quote "-param=my value" or -param="my value" or even -param=m"y v"alue. All three are correct. All three used by interpreter. I picked the oen which seems most readable. If you have another preference. I will change. |
|
May be a nit to consider - if it works, it is win win, and if it breaks anythiong, its one click to revert. So in all cases, it should do no harm. |
|
I see, if you hit it, I would have expected to see an issue raised. I have seen you raise PRs across the project that were not associated with an issue with a description that gives the reviewers context for the change (and suspected severity of the problem). Without an issue, it just looks like fidgeting. |
|
Run a grinder by changing TKG_OWNER_BRANCH to judovana:allQuoted |
|
Here we go: https://ci.adoptium.net/view/Test_grinder/job/Grinder/10761/ - failed. PR may be the cause. I have no reasons to fidget in enterprise ready project. The quoting seemed like so obvious bug with so obvious fix that I was not filling issue. Unluckily this, or even openjdk, and many other big projects share this behaviour - issues do not get attention unless they are really critical. But PRs do have attention, as there is obvious volunteer to do the work. All my PRs are based on simple effort to use that, and I keep stumbling around smaller or bigger bugs or missing features. I would say one would be happy that somebody is trying it on non default platforms and in various corenrcases:( |
|
The failure sounds like old JDK. Wil retry |
|
yah, looks better: https://ci.adoptium.net/view/Test_grinder/job/Grinder/10762/console |
|
TY! |
This PR is trying to add quotes where possible. It should not harm, but to really enbale paths with spaces may go really deep into rabbit hole