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

System.Filepath: Refactor is_windows with more documented #589

Merged
merged 3 commits into from
Jun 13, 2018
Merged

Conversation

ujihisa
Copy link
Member

@ujihisa ujihisa commented Jun 11, 2018

No description provided.

" This is tricky. has('win32') is actually enough, and you don't need
" has('win16') || has('win32') || has('win64') || has('win95')
" to tell if it's windows.
" * has('win16') does not exist.
Copy link
Member

Choose a reason for hiding this comment

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

does not exist より was removed の方が昔はあった事が分かるので良さそう?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thx! 888f750 で、両方書いてみました (現在存在しないという事実をまず明示的に表明して、その後補足的に歴史を書く流れで)

@tyru
Copy link
Member

tyru commented Jun 11, 2018

win95 やその他いにしえの feature については vim_dev の議論の結果によって変わってくるので言及してません。

派生元コメント:
#586 (comment)
vim_dev での win16 が feature-list に残っている件についてのスレッド:
https://groups.google.com/d/msg/vim_dev/owxmPRf_Pog/plmWQZLSBQAJ

@thinca
Copy link
Member

thinca commented Jun 11, 2018

本当はこの手の環境をチェックする機能(変数)のみを提供するモジュールを用意したい。
問題はモジュールロードの最中に別のモジュールを参照できないことなんだけど…。

@ujihisa
Copy link
Member Author

ujihisa commented Jun 12, 2018

https://groups.google.com/forum/#!msg/vim_dev/owxmPRf_Pog/plmWQZLSBQAJ

That's surprising. It looks like it's mostly just used to detect
MS-Windows and is used in combination of checking for win32:

    if has("win32") || has("win16") 

Since this has the highest count, let's keep this entry.

has("dos32"): 8,484 results
https:/search?l=Vim+script&q=has(%22dos32%22)+language%3A%22Vim+script%22&type=Code

has("dos16"): 8,279 results
https:/search?l=Vim+script&q=has%28%22dos16%22%29+language%3A%22Vim+script%22&type=Code

has("os2"): 5,780 results
https:/search?q=has%28%22os2%22%29+language%3A%22Vim+script%22&type=Code

All quite low, so let's omit these.

" This is tricky. has('win32') is actually enough, and you don't need
" has('win16') || has('win32') || has('win64') || has('win95')
" to tell if it's windows.
" * has('win16') does not exist; it's been removed in the past.
Copy link
Member

Choose a reason for hiding this comment

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

has('win16') and has('win95') do not exist

@tyru
Copy link
Member

tyru commented Jun 12, 2018

他のモジュールも合わせてやってもらった方がいいかなと考えたんですが、
is_windows のコメントを毎回挟むのは冗長すぎますね…
解決方法はいくつか考えられそうですが、

  1. vital 開発者 (& コントリビュータ) 向けの help (doc/vital-developer.txt) を作ってそこに書く (コードからは :help xxx で参照するだけ)
  2. Wiki に書く (URL で参照)
  3. vitalizer に便利機能を追加して定数宣言を一箇所にまとめられるようにする

@tyru
Copy link
Member

tyru commented Jun 12, 2018

既存の仕組みに乗っかるなら 2 が一番手っ取り早いかもしれません。
ここらへんに「Windows の判定には has('win32') を使うこと」ってルール追加する形で。
https:/vim-jp/vital.vim/wiki/Coding-Rule

意見募集。

@ujihisa
Copy link
Member Author

ujihisa commented Jun 12, 2018

github wikiに英語で書いて、

  • レポジトリ内の開発者向けdocを用意し、そこからリンク貼る (ことによってwikiを気づいてもらう)
  • (必要に応じて)コード内コメントがそのwiki内の特定項目にリンク貼る

の良さそう tyru++

@ujihisa
Copy link
Member Author

ujihisa commented Jun 12, 2018

@tyru e15156c で、wikiに書いてそれを参照するようにしました。

@tyru
Copy link
Member

tyru commented Jun 12, 2018

#589 (comment)

一日ぐらい待って、特に意見なければ wiki にまとめる方針で行きたいと思います…
(その場合ついでに他のモジュールの Windows 判定も直してもらいたい…)

@ujihisa
Copy link
Member Author

ujihisa commented Jun 12, 2018

期待. 他のモジュールのやつもあとで別pullreqでまとめてやるの先につくっときます。

@ujihisa
Copy link
Member Author

ujihisa commented Jun 12, 2018

作りました。本pullreqとこのpullreqは独立してreview/merge可能です
#594

@tyru tyru merged commit 9522558 into master Jun 13, 2018
@tyru tyru deleted the is_windows branch June 13, 2018 10:02
@ujihisa
Copy link
Member Author

ujihisa commented Jun 13, 2018

👏

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.

3 participants