Skip to content

vim-patch:8.1.1539,8.1.1543,8.1.1554 for :const from Vim #10313

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

Merged
merged 4 commits into from
Jun 24, 2019

Conversation

rhysd
Copy link
Member

@rhysd rhysd commented Jun 23, 2019

I've implemented :const to Vim script.

I thought these patches should be included in Neovim also for compatibility of Vim script.

This PR imports the patches related to :const to Neovim.

@rhysd
Copy link
Member Author

rhysd commented Jun 23, 2019

This is the first time to import patches into Neovim for me and almost all hunks were not automatically applied by patch command. So something may be wrong. Please let me know in the case. I'll fix it.

@marvim marvim added the vim-patch See https://neovim.io/doc/user/dev_vimpatch.html label Jun 23, 2019
@blueyed
Copy link
Contributor

blueyed commented Jun 23, 2019

@rhysd
Thanks!
You seem to have used scripts/vim-patch.sh, which is good. Patches often do not apply before other (earlier) patches are missing, or due to previous linting, and/or things not having been moved to new files (yet, or on purpose).

@rhysd
Copy link
Member Author

rhysd commented Jun 23, 2019

@blueyed Thank you for the advice. Yeah, I also fixed some lint errors.

@blueyed
Copy link
Contributor

blueyed commented Jun 23, 2019

@rhysd
See also https://github.com/neovim/nvimdev.nvim - it includes a Neomake maker, so you can do :Neomake lint also. /cc @janlazo

@rhysd
Copy link
Member Author

rhysd commented Jun 23, 2019

CI only failed on macOS with GCC. I'm investigating the failure.

[  FAILED  ]1test, listed below:
[  FAILED  ]...is/build/neovim/neovim/test/functional/core/job_spec.lua @ 599: jobs jobwait will run callbacks while waiting
...is/build/neovim/neovim/test/functional/core/job_spec.lua:622: Expected objects to be the same.

Passed in:

(table) {
  [1] = 'notification'
  [2] = 'wait'
 *[3] = {
   *[1] = 3 } }

Expected:

(table) {
  [1] = 'notification'
  [2] = 'wait'
 *[3] = {
   *[1] = 4 } }

stack traceback:
	...is/build/neovim/neovim/test/functional/core/job_spec.lua:622: in function <...is/build/neovim/neovim/test/functional/core/job_spec.lua:599>

I'm not sure why it failed, but as far as I read test/functional/core/job_spec.lua, it seems not related to this patch.

@janlazo
Copy link
Contributor

janlazo commented Jun 23, 2019

That's normal, especially for the macOS. Job and timer tests are flaky on macOS in the spite of the numerous adjustments.

My only concern is if these patches depend on older patches that haven't been (fully) ported from Vim.

@bfredl
Copy link
Member

bfredl commented Jun 23, 2019

This one is a bug in jobwait(), or rather jobwait() never implemented what the test claims it should do. Now we at least have a datapoint that #10021 didn't fix/hide the flakiness. I hope to look into fully fixing it soon.

rhysd added 3 commits June 24, 2019 09:30
Problem:    Not easy to define a variable and lock it.
Solution:   Add ":const".
vim/vim@9937a05
Problem:    Const test fails with small features.
Solution:   Don't unlet non-existing variables.
vim/vim@b6e3b88
Problem:    Docs and tests for :const can be improved.
Solution:   Improve documentation, add a few more tests. (Ryuichi Hayashida,
            closes vim/vim#4551)
vim/vim@1c196e7
@rhysd rhysd force-pushed the const-patches branch 2 times, most recently from 08d0464 to b5e3010 Compare June 24, 2019 00:35
@rhysd
Copy link
Member Author

rhysd commented Jun 24, 2019

@janlazo @bfredl

I understood the test case is flaky. Actually the test case passed in next CI job.. Let me ignore the failure in this PR.

@justinmk

Thanks for the review. I removed updates in version.c from the commits and did git push -f.

@rhysd
Copy link
Member Author

rhysd commented Jun 24, 2019

My only concern is if these patches depend on older patches that haven't been (fully) ported from Vim.

I basically agree with the concern. I took care about applying these patches not breaking something, but I'm not 100% sure. The codebase related to these patches seem almost the same as original Vim. The only non-trivial change between Neovim and Vim while applying these patch was Neovim does not implement here document for now.

const foo =<< EOS
hello,
world
EOS

Above code is valid for Vim but isn't for Neovim.

@justinmk justinmk merged commit f2e3849 into neovim:master Jun 24, 2019
@janlazo
Copy link
Contributor

janlazo commented Jun 24, 2019

@rhysd 8.1.1539 is partially merged because Neovim does not support heredoc yet.
I don't know which patch supports the following (also omitted from ported 8.1.1539 patch):

:cons[t] [{name1}, {name2}, ...] .= {expr1}

@justinmk
Copy link
Member

@janlazo that doc was later corrected in vim/vim@1c196e7

@rhysd
Copy link
Member Author

rhysd commented Jun 26, 2019

Yeah, the line was my mistake when I stole the description from let. And I fixed it later. :const cannot modify existing variables. I'm sorry for confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vim-patch See https://neovim.io/doc/user/dev_vimpatch.html
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants