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.

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

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

haya14busa
Copy link
Member

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.

return s:
endfunction

function! s:numtoname(num)
Copy link
Member

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()

Copy link
Member Author

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)
Copy link
Member

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?

@tyru
Copy link
Member

tyru commented Nov 24, 2017

Do we need to split some objects into each module?

  • VimlParser
  • VimlParser.StringReader
  • VimlParser.Compiler
  • ...

@haya14busa
Copy link
Member Author

haya14busa commented Nov 24, 2017

I don't want to introduce changes caused by making vimlparser vital-module. Users are expected to use import() method only.

@haya14busa
Copy link
Member Author

haya14busa commented Nov 24, 2017

Do we need to split some objects into each module?

We need to translate vimlparser.vim to python and javascript, so we cannot do this easily.

@tyru
Copy link
Member

tyru commented Nov 24, 2017

We need to translate vimlparser.vim to python and javascript

ah, I forgot that. thanks.

@codecov
Copy link

codecov bot commented Nov 25, 2017

Codecov Report

Merging #74 into master will increase coverage by 0.36%.
The diff coverage is n/a.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
autoload/vital/__vimlparser__/VimLParser.vim 67.3% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae76039...67cead6. Read the comment docs.

@haya14busa haya14busa force-pushed the vital-module branch 10 times, most recently from 4635ee0 to c9f5a12 Compare November 25, 2017 09:07
@haya14busa
Copy link
Member Author

Test finally passed in travis CI! (Thanks to @thinca 👍)

It's ready to review and merge. Can you take a look at this p-r? @mattn @tyru

@tyru
Copy link
Member

tyru commented Nov 25, 2017

  • Change to autoload/vital/__vimlparser__/VimlParser.vim ? or remove them?

verbose echo 's:VimLParser.builtin_commands in autoload/vimlparser.vim is up-to-date.'

verbose echo "Append following lines to s:VimLParser.builtin_commands in autoload/vimlparser.vim\n"

  • is it confusing that module name is VimlParser, but class(object) name is VimLParser?

@haya14busa
Copy link
Member Author

Thanks! Done.

@mattn
Copy link
Member

mattn commented Nov 25, 2017

Currently, VimLParser seems to export following functions. Is this your expected?

isalnum
Node
ExArg
islower
isnamec
iswordc1
isdigit
iswhite
import
isvarname
isalpha
Err
isnamec1
isargname
isupper
isxdigit
iswordc
isodigit
isidc

@haya14busa
Copy link
Member Author

Yes, it's expected. I don't want to introduce changes caused by making it vital-module as much as possible.
Also, the changes will be translated into other languages too and i feel a bit weird to add _ prefix to all script local functions.

s:import() should be only public API. I'll add comment.

@haya14busa
Copy link
Member Author

s:import() should be only public API. I'll add comment.

Done. 67cead6

@@ -14,7 +14,7 @@ This parser provide same feature for following languages.

* Vim script
* Python
* Javascript
* JavaScript

## Example

Copy link
Member

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

Copy link
Member

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

Copy link
Member

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()

Copy link
Member

Choose a reason for hiding this comment

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

🍣

@ujihisa
Copy link
Member

ujihisa commented Nov 26, 2017

Besides that I believe this LGTM 🍣 as a vim-jp org but I prefer listening to other core devs voices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants