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.

Sign up for GitHub

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

Jump to bottom

Replace Python core with pure-Vimscript core ("vimcore") #119

Merged
merged 8 commits into from
May 21, 2019
Merged

Replace Python core with pure-Vimscript core ("vimcore") #119

merged 8 commits into from
May 21, 2019

Conversation

cxw42
Copy link
Member

@cxw42 cxw42 commented Apr 19, 2019

Here it is - let me know what you think! Passes all tests except Windows UTF-8, which @ppalaga says might not be possible (editorconfig/editorconfig-core-c#31 (comment)).

Closes editorconfig/editorconfig#383 .
Closes #48 (replaces it).

Also closes the open Python issues:
Closes #57.
Closes #101.
Closes #116.

May help with #108.
May affect #111.

Part of the discussion in vim/vim#2286 .

@cxw42
Copy link
Member Author

cxw42 commented Apr 20, 2019

The CI failure appears to be related to rake/bundler/vimrunner versions. Would someone with some Ruby expertise be able to give me some suggestions?

@xuhdev
Copy link
Member

xuhdev commented Apr 20, 2019

I guess you have generated the lock file in a new version of bundler. The easiest way to resolve this is probably use bundler less than 2.0 to generate Gemfile.lock on your system.

cxw42 pushed a commit to cxw42/editorconfig-vim that referenced this pull request Apr 22, 2019
Revert "Specify rake dependencies so tests can run"
bee10bc 
This reverts commit e1c2557.

Thanks to
editorconfig#119 (comment)
for the suggestion.

All tests now pass with `bundle _1.15.4_ exec rake`.
@cxw42
Copy link
Member Author

cxw42 commented Apr 22, 2019

@xuhdev Thanks - that worked! I was able to run the tests on Cygwin from editorconfig-vim/ with

export BUNDLE_GEMFILE=tests/Gemfile
bundle _1.15.4_ exec rspec tests/spec/editorconfig_spec.rb

Please let me know if you would like any changes, or if you have any concerns.

@cxw42
Copy link
Member Author

cxw42 commented Apr 26, 2019

@xuhdev Any thoughts? I am wondering if we can merge this one. After that, I would like to work with @rakus to see if we can merge some of his plugin changes into this official plugin. I think removing the Python should come before any Vimscript enhancements. What say you?

xuhdev
xuhdev reviewed Apr 26, 2019
View reviewed changes
Copy link
Member

@xuhdev xuhdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have CI set up?

@xuhdev
Copy link
Member

xuhdev commented Apr 26, 2019

I remember you said something about number range ({-3...100}) and this core depends on that. I wanted to start a voting for that I couldn't find that thread. Could you remind me where it is?

@cxw42
Copy link
Member Author

cxw42 commented Apr 28, 2019

@xuhdev

CI

The CI (Travis) is unchanged in this PR. The core is copied from https://github.com/cxw42/editorconfig-core-vimscript , which has its own CI (Appveyor) set up.

number range

The number range is editorconfig/editorconfig#371 . Thank you for following up on it!

I changed this PR so it does not depend on that issue. It has the current behaviour (e.g., {-10..10} rejects 0). Therefore, I think this can go ahead even before the vote on number ranges.

@xuhdev
Copy link
Member

xuhdev commented Apr 28, 2019

I understand you haven't changed anything about CI. What I meant is that, if you add a core library to the repo, we need CI to test that core library for guaranteeing its continuously correct functioning...

@cxw42
Copy link
Member Author

cxw42 commented Apr 28, 2019

@xuhdev Sorry for misunderstanding! I have not but am happy to. However, the vimscript core requires a number of support files in order to be testable by ctest (e.g., bash scripts and bat files). May I add a separate repo submodule for those so plugin users don't get files they don't need?

@xuhdev
Copy link
Member

xuhdev commented Apr 28, 2019

I personally feel it's OK to add editorconfig/editorconfig-core-test as a submodule, but all other scripts IMO are integrated part of this subproject. Users in general get testing scripts, same for most other cases if they are supposed to get things from source. In addition, test scripts are unlikely to use a lot of spaces anyway.

@cxw42
Copy link
Member Author

cxw42 commented Apr 28, 2019

@xuhdev OK - I'll update this repo. I may ask to revisit this point later if Bram decides he's willing to bundle this plugin with Vim.

cxw42 pushed a commit to cxw42/editorconfig-vim that referenced this pull request Apr 28, 2019
@cxw42
Revert "Specify rake dependencies so tests can run"
b7f348b 
This reverts commit e1c2557.

Thanks to
editorconfig#119 (comment)
for the suggestion.

All tests now pass with `bundle _1.15.4_ exec rake`.
@cxw42 cxw42 force-pushed the vimcore branch 16 times, most recently from c4c183e to 5079f24 Compare April 30, 2019 14:59
@cxw42 cxw42 force-pushed the vimcore branch 2 times, most recently from b3126ad to 6549e37 Compare May 8, 2019 13:57
xuhdev
xuhdev reviewed May 17, 2019
View reviewed changes
Copy link
Member

@xuhdev xuhdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding to your new changes, I've made another comment. You also seem to have changed EOL of a few files. Just wanna make sure you intended it. After resolving this comment, it should be fine to merge now. We can make adjustment later once the specification version format is determined.

autoload/editorconfig_core.vim Outdated Show resolved Hide resolved
@cxw42
Copy link
Member Author

cxw42 commented May 19, 2019

@xuhdev #119 (review)

Regarding to your new changes, I've made another comment. You also seem to have changed EOL of a few files. Just wanna make sure you intended it.

Would you please let me know which files you are looking at? I just looked through git diff upstream/master and didn't see any EOL changes. All files currently have Unix (LF) line endings, which I think is reasonable. (Except for the crlf.in file from editorconfig-core-test, which needs to have CRLF to function! :) )

There are a number of renames. For example, I moved tests/ to tests/plugin so that I could create tests/core. Is that perhaps what you are looking at?

@xuhdev
Copy link
Member

xuhdev commented May 20, 2019

Perhaps I'm getting confused. Could you tell me, for example, what has changed in tests/core/ecvimlib.ps1 since then? I've seen a whole file change here.

@cxw42
Copy link
Member Author

cxw42 commented May 20, 2019 •
edited
Loading

Ah! Sure. Compared to master, that is a new file. Everything intests/core is copied in from editorconfig-core-vimscript per your earlier request to combine the repos. But I see that GitHub, at least on mobile, does not make that clear at all!

I'll take a look at that specific commit. I think I was using crlf in the other repo and regularized it to lf here. Edit That is indeed the case - I was using CRLF for files on Windows systems while developing the core. My thought at the time was that Windows users who opened Windows-specific files in Notepad would be able to read those files if they were CRLF. I will change those back and clean up the history a bit.

Chris White and others added 7 commits May 20, 2019 06:56
@cxw42
Added support for Vimscript core
c151747 
- Added "vim_core" mode.
- Use the Vimscript core as the default if it is installed and the user
  has not specified a different core.
- Also, for robustness, accept core modes case-insensitively.
- Added VimScript core to documentation
@cxw42
Copied in editorconfig-core-vimscript
7051a8d 
Per @xuhdev in editorconfig/editorconfig#383 (comment)

From cxw42/editorconfig-core-vimscript@4495efd

Removed caching code, which is not yet used, per @xuhdev in
editorconfig#119 (comment)
@cxw42
Remove support for all cores but vimscript
f5adea3 
- Remove the Python core from this plugin
- Remove all plugin code related to modes other than vim_core
- Update the documentation and README accordingly

Note on tests: All tests now pass with `bundle _1.15.4_ exec rake`.
Thanks to
editorconfig#119 (comment)
for the suggestion on how to make this work.
@cxw42
Add commands to enable/disable plugin
9059c63 
:EditorConfigEnable, :EditorConfigDisable
@cxw42
Moved plugin tests from tests/ to tests/plugin/
62e60a0 
- This makes room for adding core tests.
- Also, renamed and updated plugin_tests submodule
@cxw42
Added core_tests submodule and scripts
1b3c91f 
Scripts are modified from cxw42/editorconfig-core-vimscript@5d85d10

Windows-specific files have CRLF line endings; other files have LF
line endings.
@cxw42
Core tests in Travis; added Appveyor
b45e192 
- Added the core tests to the existing Travis configuration

- Added Appveyor configuration to permit testing on Windows.
  The configuration is modified from
  cxw42/editorconfig-core-vimscript@5d85d10

- Updated the README to include both badges.  For now, they point to
  cxw42/editorconfig-vim; that will need to be updated when this branch
  is merged.
@cxw42
Copy link
Member Author

cxw42 commented May 20, 2019

@xuhdev I cleaned up and squashed some. The CRLF files are now consistent throughout the commits in which they occur, and the .editorconfig file specifies those line ends appropriately. The previous commits are still available in cxw42/vimcore-old if you want to compare.

@cxw42
Updated license and documentation
7d48003 
Also, fixed typo in Appveyor configuration.
@xuhdev xuhdev merged commit 37bedf8 into editorconfig:master May 21, 2019
@xuhdev
Copy link
Member

xuhdev commented May 21, 2019

Thank you for your hard work!

@cxw42
Copy link
Member Author

cxw42 commented May 21, 2019

@xuhdev Happy to help!

When you have a chance, would you please set up Appveyor for this repo under editorconfig and update README.md? It currently points to my Appveyor account. No hurry, but I think it would be a good idea to continue running those tests.

@xuhdev
Copy link
Member

xuhdev commented May 21, 2019

Done!

@xuhdev
Copy link
Member

xuhdev commented Jun 4, 2019

Sorry for the late discussion on this issue, but I realized that we should have retained the ability to use an external core library as an option (the one that calls the external editorconfig executable, not the Python builtin). Could you restore the availability of that option, if this is not too difficult? We have done something similar to Emacs before: editorconfig/editorconfig-emacs#46 (comment) I'm OK with defaulting to the internal one, but an option to use an external one might still be important.

This was referenced Jun 5, 2019
Closed
Closed
@krishnakumarg1984 krishnakumarg1984 mentioned this pull request Jun 24, 2019
Closed
tony added a commit to tony/vim-config-framework that referenced this pull request Feb 23, 2020
@tony
Use editorconfig/editorconfig-vim again
4d3f47a 
Lots of updates, including moving to pure vimscript (vimcore)

See also: editorconfig/editorconfig-vim#119
@cxw42 cxw42 deleted the vimcore branch October 30, 2022 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ffes ffes ffes left review comments

@xuhdev xuhdev xuhdev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@cxw42 @xuhdev @ffes