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

Add the supports to use Funcref and Data.Closure's {callable} in Data.List #511

Merged
merged 90 commits into from Jun 9, 2018

Conversation

Projects
None yet
6 participants
@aiya000
Contributor

aiya000 commented May 18, 2017

  • Data.List will support Funcref
    • {callable} is not supported in here
  • Data.List.Closure will support {callable}
    • Data.List.Closure provides to call Data.List's functions
\ : execute(printf("throw 'vital: Data.List: sort_by() cannot compare %s and %s'", string(x), string(y)), 1)
endfunction
return s:sort(a:list, function(scope.compare_with, [], scope))

This comment has been minimized.

@vim-jp-bot

vim-jp-bot May 18, 2017

[vimlint] reported by reviewdog 🐶
E118: Too many arguments for function: function

" Lifts the string expression to the function.
" s:sort_expr must be defined as the string expression of the binary function
" before this is called.
function! s:_compare_by_string_expr(a, b) abort

This comment has been minimized.

@vim-jp-bot

vim-jp-bot May 18, 2017

[vimlint] reported by reviewdog 🐶
EVL103: unused argument a:a

" Lifts the string expression to the function.
" s:sort_expr must be defined as the string expression of the binary function
" before this is called.
function! s:_compare_by_string_expr(a, b) abort

This comment has been minimized.

@vim-jp-bot

vim-jp-bot May 18, 2017

[vimlint] reported by reviewdog 🐶
EVL103: unused argument a:b

" s:sort_expr must be defined as the string expression of the binary function
" before this is called.
" a:a and a:b are used in s:sort_expr .
function! s:_compare_by_string_expr(a, b) abort

This comment has been minimized.

@vim-jp-bot

vim-jp-bot May 18, 2017

[vimlint] reported by reviewdog 🐶
EVL103: unused argument a:b

" s:sort_expr must be defined as the string expression of the binary function
" before this is called.
" a:a and a:b are used in s:sort_expr .
function! s:_compare_by_string_expr(a, b) abort

This comment has been minimized.

@vim-jp-bot

vim-jp-bot May 18, 2017

[vimlint] reported by reviewdog 🐶
EVL103: unused argument a:a

@aiya000 aiya000 added the WIP label May 19, 2017

" Notice:
" This is not job safe.
" The function may not work correctly.

This comment has been minimized.

@tyru

tyru May 19, 2017

Member

Data.Closure なら binding で変数渡せそう?

This comment has been minimized.

@aiya000

aiya000 May 19, 2017

Contributor

ここの目的は{closure}をFuncrefに変換することなので、{binding}使うとData.Listの各関数に{closure}渡すことになってしまうような?

This comment has been minimized.

@tyru

tyru May 20, 2017

Member

はい、closure 的なことがやりたければ、Vim 7.4 だったら Data.Closure オブジェクトみたいな dictionary を持ちまわるしかないと思います

This comment has been minimized.

@aiya000

aiya000 May 20, 2017

Contributor

それだとData.Listの関数がClosure対応をしてないのでだめな気がする……。(Data.Closure遅かったので、削除して回ってる)

This comment has been minimized.

@tyru

tyru May 20, 2017

Member

あ、なるほど、そういう方針でしたか…

@aiya000

This comment has been minimized.

Contributor

aiya000 commented May 19, 2017

辞書の寿命が切れる…… > wip

@tyru

This comment has been minimized.

Member

tyru commented Jun 8, 2018

:help Vital.Data.Closure-term-callable より。
これ冒頭のコメントに載せると {callable} が何かすぐ分かって便利そうです。

{callable}                              Vital.Data.Closure-term-callable
        A {callable} is one of the followings.
        - A Funcref
          - Vital.Data.Closure.from_funcref()
        - A string of function name which begins from "*"
          - Vital.Data.Closure.from_funcname()
        - An ex command string which begins from ":"
          - Vital.Data.Closure.from_command()
        - A List of ex command strings which begin from ":"
          - Vital.Data.Closure.from_command()
        - An operator string such as '+', '%', or '=~#'
          - Vital.Data.Closure.from_operator()
        - An expr string which begins from "="
          - Vital.Data.Closure.from_expr()
        - A Closure object(Vital.Data.Closure-Closure)
        - A List that is arguments of Vital.Data.Closure.build().
@tyru

This comment has been minimized.

Member

tyru commented Jun 8, 2018

この PR の概要

間が空いてしまったので一旦自分のためにも整理します (ほぼ PR 本文と同じですが日本語&思い出す意味でも…)

後で修正したい点 (既存実装の問題、Vim 7.4 サポート終了、...)

既存実装とかの事まで言ってたらキリがないので、とりあえずこの PR では request changes はしないでおきますが、備忘録として。
とりあえず私がこの PR で見たのは Data.List の関数が Funcref を受け取れるようになっていることぐらいです…
(すみません、Data.List.Closure 側はほぼ見てません…)

8.1 が出て 7.4 サポートは打ち切る事になった (#580) ので今だと

  • sort(){dict} 引数が使える (そもそも List.sort_by() 使わないかも…)
  • :for の中で型が違ってもエラーにならないので :unlet しなくてもいい
  • lambda, func-closure が使える

とかあるのでもうちょっと短く書ける部分もありそうです。

上記以外で、その他にも色々と直したい部分 (既存の Data.List の問題)

  • s:concat() のテストがない (元から)
  • s:char_range() のテストがない (元から)
  • s:has() のテストがない (元から)
  • s:has_index() のテストがない (元から)
  • s:or() のテストの :Describe の引数が .and() になってる (恐らくコピペミス)
  • テストの定義位置 (:Describe) を実装側と合わせる
  • s:binary_search() がオーバーフローして正しい結果を返さない
    • 良くある間違いの一つは、imin + (imax - imin) / 2 を (imax + imin) / 2 としてしまう事である (Wikipedia - 二分探索)
@@ -3,6 +3,15 @@
let s:save_cpo = &cpo
set cpo&vim
function! s:_vital_loaded(V) abort
let s:Closure = a:V.import('Data.Closure')

This comment has been minimized.

@tyru

tyru Jun 8, 2018

Member

使ってなさそう

@tyru

This comment has been minimized.

Member

tyru commented Jun 8, 2018

Data.List が Data.Closure に依存しちゃってるの直したらマージしちゃって良さそう (と個人的には思いますがどうでしょう >ALL)

@ujihisa

This comment has been minimized.

Member

ujihisa commented Jun 8, 2018

I agree with tyru. Let's remove Data.Closure dependency from Data.List and merge this pullreq.

@ujihisa

This comment has been minimized.

Member

ujihisa commented Jun 9, 2018

優勝感

@aiya000

This comment has been minimized.

Contributor

aiya000 commented Jun 9, 2018

優勝を完了しました(多分)!

@tyru まとめてくださって助かりました、ありがとうございます!

@tyru

tyru approved these changes Jun 9, 2018

@tyru

This comment has been minimized.

Member

tyru commented Jun 9, 2018

LGTM

@ujihisa

This comment has been minimized.

Member

ujihisa commented Jun 9, 2018

It took over one year. Congratulations for merging!

@tyru tyru merged commit 6efcf7d into vim-jp:master Jun 9, 2018

5 checks passed

codecov/patch 89.39% of diff hit (target 78.47%)
Details
codecov/project 78.78% (+0.3%) compared to a8773a3
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tyru

This comment has been minimized.

Member

tyru commented Jun 9, 2018

@aiya000 ありがとうございました!

@ujihisa

This comment has been minimized.

Member

ujihisa commented Jun 10, 2018

これせっかくなので周知されてこそ意義あると思うし、作者直々の使い方解説ブログ記事とかがあると、すごい便利そうです!

@aiya000 aiya000 deleted the aiya000:add_funcref_impl_datalist branch Jun 10, 2018

@aiya000

This comment has been minimized.

Contributor

aiya000 commented Jun 10, 2018

うおおおお、やったー!!
ありがとうございましたー!!

@ujihisa
確かに…やります!
ご支援ありがとうございました!

@tyru
長らく支援していただいて、またまとめ等、大変ありがとうございました!

@Milly
詰まってたところを修正していただけて、本当にありがたかったです 🙏
必要不可欠でした。
おかげさまでマージされました、ありがとうございました!!

@ujihisa

This comment has been minimized.

Member

ujihisa commented Jun 10, 2018

期待

@aiya000

This comment has been minimized.

Contributor

aiya000 commented Jun 10, 2018

@ujihisa

This comment has been minimized.

Member

ujihisa commented Jun 10, 2018

感極まってきまし (た)

@tyru

This comment has been minimized.

Member

tyru commented Jun 10, 2018

よっ(しゃ)

@Milly

This comment has been minimized.

Contributor

Milly commented Jun 11, 2018

Data.List の各関数って引数のリストに対して非破壊的ですよね?
(ちゃんと全部確認してない。)

というわけで TODO

  • テストで copy() を使っている箇所を消す。
  • 非破壊的であることをテストする。
  • ドキュメントで copy() を使っている箇所を消す。
  • ドキュメントに nondestructively との説明を追加する。
    • Data.List.uniq(), uniq_by() のみ、すでに記載されている。
@ujihisa

This comment has been minimized.

Member

ujihisa commented Jun 19, 2018

@Milly Data.Listのpop(), push(), shift(), unshift(), clear()などは意図的に破壊的です。

うーんこれどうしようかな

  • A. immutableなやつとmutableなやつで2つに分ける (scalaみたいに)
    • Data.Listはimmutableで、Data.List.Mutableはmutableみたいな
  • B. mutableなやつはその目的に応じて別のデータ構造とする
    • Stack.pop()/push()とか、Queueとか
  • C. わけないで全部Data.Listに残すが、関数ごとにdocumentで破壊的・非破壊的か明示する
  • D. わけないで全部Data.Listに残すが、関数ごとに関数名で破壊的・非破壊的か分かりやすくする

書いててあれだけど、個人的にvitalではA, Bは良くないと思います。細かく分かれすぎると結局ユーザにとって分かりにくいかなと。Dにする場合、どういうのがいいかなあ. 思いつかなければCが自動優勝

@aiya000

This comment has been minimized.

Contributor

aiya000 commented Jun 19, 2018

Cよさそう!

@tyru

This comment has been minimized.

Member

tyru commented Jun 19, 2018

とりあえずドキュメントに書くのは必須ですね。

@ujihisa

This comment has been minimized.

Member

ujihisa commented Jun 19, 2018

とりあえずのCのためのpullreq作っておきます #592 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment