-
Notifications
You must be signed in to change notification settings - Fork 26
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
Create VimlParser vital-module #74
base: master
Are you sure you want to change the base?
Conversation
return s: | ||
endfunction | ||
|
||
function! s:numtoname(num) |
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.
s:numtoname()
is used by vimlparser#test()
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.
Thanks. Done
|
||
let s:MAX_FUNC_ARGS = 20 | ||
|
||
function! s:isalpha(c) |
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.
These functions are really needed to be exported?
Do we need to split some objects into each module?
|
3c6100b
to
bb0dbdb
Compare
7e99f4e
to
02306ef
Compare
I don't want to introduce changes caused by making vimlparser vital-module. Users are expected to use import() method only. |
We need to translate vimlparser.vim to python and javascript, so we cannot do this easily. |
ah, I forgot that. thanks. |
Codecov Report
@@ Coverage Diff @@
## master #74 +/- ##
=========================================
+ Coverage 66.93% 67.3% +0.36%
=========================================
Files 1 1
Lines 3590 3569 -21
=========================================
- Hits 2403 2402 -1
+ Misses 1187 1167 -20
Continue to review full report at Codecov.
|
4635ee0
to
c9f5a12
Compare
a561e12
to
b2fb2c9
Compare
|
Thanks! Done. |
Currently, VimLParser seems to export following functions. Is this your expected? isalnum |
Yes, it's expected. I don't want to introduce changes caused by making it vital-module as much as possible.
|
Done. 67cead6 |
@@ -14,7 +14,7 @@ This parser provide same feature for following languages. | |||
|
|||
* Vim script | |||
* Python | |||
* Javascript | |||
* JavaScript | |||
|
|||
## Example | |||
|
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.
Should the L24 below updated like this?
before:
let s:vimlparser = vimlparser#import()
after:
let s:V = vital#yourpluginname#new()
let s:vimlparser = s:V.import('VimLParser')
with vitalize
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.
👍 for writing vital API instead of autoload API.
And your "after" code should be:
let s:vimlparser = vital#yourpluginname#import('VimLParser').import()
According to https://github.com/vim-jp/vim-vimlparser/pull/74/files#diff-ec29e185fa61087ecf991ff8811a8acaR6
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.
In the first place, current example code is wrong...
-let r = s:StringReader.new(code)
-let p = s:VimLParser.new()
-let c = s:Compiler.new()
+let r = s:vimlparser.StringReader.new(code)
+let p = s:vimlparser.VimLParser.new()
+let c = s:vimlparser.Compiler.new()
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.
🍣
Besides that I believe this LGTM 🍣 as a vim-jp org but I prefer listening to other core devs voices. |
Create vital-module interface of vimlparser.
See https://github.com/vim-jp/vital.vim for vital-module.
By introducing vital-module, users can use vimlparser as vital-module and we can mitigate the migration problem of breaking changes.