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

Allow use of function references as callbacks #1067

Merged
merged 1 commit into from
Dec 15, 2019
Merged

Allow use of function references as callbacks #1067

merged 1 commit into from
Dec 15, 2019

Conversation

HiPhish
Copy link
Contributor

@HiPhish HiPhish commented Dec 13, 2019

Description of Changes

Closes #1066


New Version Info

Allow use of function references as callbacks instead of only strings in menu API. This approach could be extended in the future to support function references for all callbacks.

Author's Instructions

  • Derive a new MAJOR.MINOR.PATCH version number. Increment the:
    • MAJOR version when you make incompatible API changes
    • MINOR version when you add functionality in a backwards-compatible manner
    • PATCH version when you make backwards-compatible bug fixes
  • Update CHANGELOG.md, following the established pattern.

Collaborator's Instructions

  • Review CHANGELOG.md, suggesting a different version number if necessary.
  • After merge, tag the merge commit, e.g. git tag -a 3.1.4 -m "v3.1.4" && git push origin --tags

@PhilRunninger
Copy link
Member

I like this. While you're at it could you similarly update the NERDTreeAddKeyMap() API function? The function that you'll have to change is: https://github.com/scrooloose/nerdtree/blob/master/lib/nerdtree/key_map.vim#L66-L75

@HiPhish
Copy link
Contributor Author

HiPhish commented Dec 13, 2019

@PhilRunninger Done, I also did it for paths (https://github.com/scrooloose/nerdtree/blob/master/lib/nerdtree/path.vim#L503). Let me know if there are any more places to patch. I'm feeling under the weather right now and I'm not in the mood to go digging through the code, but if one can tell me approximately what to look for I can do it.

@PhilRunninger
Copy link
Member

Looks like there's one more here: https://github.com/scrooloose/nerdtree/blob/master/lib/nerdtree/notifier.vim#L18

Also , make sure you update the CHANGELOG.md. Use version number 6.4.0, since this adds some new functionality.

@HiPhish
Copy link
Contributor Author

HiPhish commented Dec 13, 2019

@PhilRunninger Done.

Copy link
Member

@PhilRunninger PhilRunninger left a comment

Choose a reason for hiding this comment

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

Just a couple small changes left.

  1. I ran through a few tests of the code to check for errors. I found a couple places where a variable needs to be made uppercase.
  2. I like the compactness of the ternary operator, and would like to see it throughout your code changes.
  3. A minor formatting change to the change log.

CHANGELOG.md Outdated
@@ -7,6 +7,8 @@
in an unordered list. The format is:
- **.PATCH**: Pull Request Title (PR Author) [PR Number](Link to PR)
-->
#### 6.4
- .0: Allow use of function references as callbacks (HiPhish) [#1067](https://github.com/scrooloose/nerdtree/pull/1067)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- .0: Allow use of function references as callbacks (HiPhish) [#1067](https://github.com/scrooloose/nerdtree/pull/1067)
- **.0**: Allow use of function references as callbacks (HiPhish) [#1067](https://github.com/scrooloose/nerdtree/pull/1067)

@@ -66,7 +66,11 @@ endfunction
"FUNCTION: KeyMap.invoke() {{{1
"Call the KeyMaps callback function
function! s:KeyMap.invoke(...)
let Callback = function(self.callback)
if type(self.callback) == v:t_func
Copy link
Member

Choose a reason for hiding this comment

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

Replace this if block with the ternary operator assignment below.

    let Callback = type(self.callback) == v:t_func ? self.callback : function(self.callback)

@@ -79,7 +79,11 @@ endfunction
"specified
function! s:MenuItem.enabled()
if self.isActiveCallback != -1
return {self.isActiveCallback}()
if type(self.isActiveCallback) == v:t_func
Copy link
Member

Choose a reason for hiding this comment

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

Replace this if block with the ternary operator assignment below.

        return type(self.isActiveCallback) == v:t_func ? self.isActiveCallback() : {self.isActiveCallback}()

@@ -15,7 +15,12 @@ function! s:Notifier.NotifyListeners(event, path, nerdtree, params)
let event = g:NERDTreeEvent.New(a:nerdtree, a:path, a:event, a:params)

for listener in s:Notifier.GetListenersForEvent(a:event)
call {listener}(event)
if type(listener) == v:t_func
Copy link
Member

Choose a reason for hiding this comment

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

listener needs to be capitalized here and in line 17. Replace the if block with this ternary operator assignment.

    for Listener in s:Notifier.GetListenersForEvent(a:event)
        let Callback = type(Listener) == v:t_func ? Listener : function(Listener)

@@ -501,7 +501,8 @@ function! s:Path.ignore(nerdtree)
endfor

for callback in g:NERDTree.PathFilters()
if {callback}({'path': self, 'nerdtree': a:nerdtree})
let Callback = type(callback) == v:t_func ? callback : function(callback)
Copy link
Member

Choose a reason for hiding this comment

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

callback needs to be capitalized here and in line 503.

        for Callback in g:NERDTree.PathFilters()
            let Callback = type(Callback) == v:t_func ? Callback : function(Callback)

@HiPhish
Copy link
Contributor Author

HiPhish commented Dec 14, 2019

@PhilRunninger Done.

I like the compactness of the ternary operator, and would like to see it throughout your code changes.

I like it as well, but I thought that the lines would end up being too long, so I had opted for if-blocks instead. I have changed it to the ternary operator now.

A minor formatting change to the change log.

I think I should turn off concealing for Markdown buffers, at least if the buffer is not nofile. I was wondering why none of the other changelog entries had ** either.

@PhilRunninger PhilRunninger merged commit a7886fb into preservim:master Dec 15, 2019
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.

Use function references (funcref) as callbacks?
2 participants