-
Notifications
You must be signed in to change notification settings - Fork 139
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
Replace Python core with pure-Vimscript core ("vimcore") #119
Conversation
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? |
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. |
This reverts commit e1c2557. Thanks to editorconfig#119 (comment) for the suggestion. All tests now pass with `bundle _1.15.4_ exec rake`.
@xuhdev Thanks - that worked! I was able to run the tests on Cygwin from
Please let me know if you would like any changes, or if you have any concerns. |
There was a problem hiding this 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?
I remember you said something about number range ( |
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.
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., |
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... |
@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? |
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. |
@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. |
This reverts commit e1c2557. Thanks to editorconfig#119 (comment) for the suggestion. All tests now pass with `bundle _1.15.4_ exec rake`.
c4c183e
to
5079f24
Compare
b3126ad
to
6549e37
Compare
There was a problem hiding this 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.
Would you please let me know which files you are looking at? I just looked through There are a number of renames. For example, I moved |
Perhaps I'm getting confused. Could you tell me, for example, what has changed in |
Ah! Sure. Compared to 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. |
- 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
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)
- 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.
:EditorConfigEnable, :EditorConfigDisable
- This makes room for adding core tests. - Also, renamed and updated plugin_tests submodule
Scripts are modified from cxw42/editorconfig-core-vimscript@5d85d10 Windows-specific files have CRLF line endings; other files have LF line endings.
- 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.
@xuhdev I cleaned up and squashed some. The CRLF files are now consistent throughout the commits in which they occur, and the |
Also, fixed typo in Appveyor configuration.
Thank you for your hard work! |
@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. |
Done! |
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. |
Lots of updates, including moving to pure vimscript (vimcore) See also: editorconfig/editorconfig-vim#119
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 .