doge#helpers#trim (vim version compatibility) #42
Conversation
An helper function that can replace the trim() function introduced in patch 7.4.1630. It guarantees compatibility with at least vim 8 (debian stretch).
|
What vim version are you running? |
|
I'm on debian stretch at the moment, so 8.0, but without the 1630 patch that has the trim() function. |
|
@mg979 That's still no answer to my question. What Vim version are you using? I specifically need the patch number. Open Vim and check the startup screen. |
|
|
What specific Vim version is your Vim mentioning in your startup screen when you run |
|
Yes, 8.0.707 |
|
I will look into it later. Thanks for the effort of the PR. Also, I did merge master in your branch, since I noticed that I didn't include Vim 8.0 in the travis tests, which is highly critical for this PR. |
|
Ok thanks. There's another PR that is more involving (and more important I think). One test is failing though, could be because of the last commit. |
|
I suggest you pull from the master branch and push that to your second pull request its branch. |
|
Can you figureout which Vim version (including patch) is required now besides 8.0.1630? Otherwise I will test this. |
|
I don't know how I could test it, but at least saying >800 would be safe I think. |
|
I use the docker to built different images and then I test hem individually. I'll do that later today, thanks anyway. |
Minimum versions set to: - vim: 7.4.2219 - nvim: 0.2.0
Put caret, remove \r and \n
|
Ugh, tests are failing. I'll fix this tomorrow. Probably we have to go higher than Vim 7.4.2119. I'll check it tomorrow. |
|
It could be because of the different pattern, a test is failing after that change, and it was ok before. I can try to revert that commit and see if it fixes it. |
|
Tests run fine now, it could be the caret, is it ok if I force-push some patterns and see what's the reason? |
Add caret but keep \r and \n (because of failing tests with d5b7f76)
|
I added the caret but restored |
|
Yes I get the unsupported message on 7.4. |
Which patch version? |
|
It's in 7.4 with no patches applied (welcome screen shows just 7.4). |
|
@mg979 Do you have anything else? I think we can merge into master. |
|
I was saying, maybe reword last commit and force-push 😃 ( Now just saying (hoping I'm not sounding critic), but hyper-optimizing things can be harmful, last commit for example, vim is sensitive about quote types for patterns and I couldn't be sure if printf() returns the right kind of quote, you could have used string concatenation but it's a waste of time mostly. I would just revert the last commit unless you're sure there's some real gain from it (I don't think there is). |
It seems that it does. Tests are doing perfect. |
|
Mhm, I tried to |
|
@mg979 Shall I merge? |
|
For me it's ok, just one thing: how is it that tests didn't catch the trim() error before (from the missing dash in the patch version check)? |
"the trim() error" as in: the error that you get in version 7? |
|
Can you provide the full version number that you've tested? |
|
Still the same as in |
|
The trim() function is supported since 8.0.1630+ and I did not have tests for below this version. You used a version where it wasn't supported yet and I'm not going to test a version that is below the minimum supported version. The docker versions all supported the trim() function so it didn't error on that, but we fixed that now since we support 7.4. |
|
Understood, the important thing is that tests work as intended. For me it's ok! |
|
This feature has been merged and released in v1.12.0. Feel free to submit any new issues if you experience any unwanted behavior in the future. Thanks for your contribution. |
Prelude
Thank you for helping out DoGe!
By contributing to DoGe you agree to the following statements (Replace
[ ]with[x]with those you agree with):Why this PR?
Compatibility with older vim version.
How older? I don't know, but it works with vim 8 debian stretch.