mozilla-japan / dev.mozilla.jp

dev.mozilla.jp のテーマ
1 stars 1 forks source link

コード整形 #40

Open ethertank opened 11 years ago

ethertank commented 11 years ago

コードの書式にブレがあり、更新の妨げとなる為、これらを一括して統一しています。

また、一部視認性向上を目的とした好みによる改行の 追加/ 削除 も行っています。

tetsuharuohzeki commented 11 years ago

https://github.com/ethertank/modest_themes/commit/41f8e9cc2f4100431d2fee271dbb17a09debe6c0 にて、結構な数のファイルが全面書き換えになっていますが、なぜですか? (例: author.php )

  • HTML の空要素 ( input / br ) 末尾のスラッシュ前のスペースを「有り」に統一。
  • CSS ブロック最終行末尾のセミコロンを「有り」に統一。
  • PHP の関数ブロック前のスペースを「有り」に統一。
  • PHP の else / if else などの前後の改行を「無し」に統一。前後のスペースを「有り」に統一。

のルールではこのような大規模変更は考え難いのですが;

ethertank commented 11 years ago

概ね申し上げた通りの変更しか行っていない筈です。

tetsuharuohzeki commented 11 years ago

の変更は、改行コードが意図せず変更されてしまっているのが原因です。git amendなどで改行コードを元に戻してください。

ethertank commented 11 years ago

CSS ファイルがどの改行コードを使用しているか分からず、 "CR" 、 "LF" 、 "CR LF" を試しましたが、どの場合でも「全面書き換え」のように表示されました。結局、 mozilla-japan / modest_themes の master のファイルをエディタで確認し、"CR LF" のようだったのでこれに戻しています。

tetsuharuohzeki commented 11 years ago

結局、 mozilla-japan / modest_themes の master のファイルをエディタで確認し、"CR LF" のようだったのでこれに戻しています。

私の手元で mozilla-japan / modest_themes:masterのファイルを確認した所、改行コードはLF(UNIX)のものが多数。一部ファイルのみがCR LF(Windows)という状況でした。

食い違いがあるようなので、http://www.kashim.com/kanjitranslator/index.html などを使って、確認してもらえますか?

私の確認した所、CR+LFだったものは

です。また、同時に調べてみましたが、改行コードに限らず、さらに一部の文字エンコードがutf-8ではないのも確認しました。 この件については、別途issueを立て、改行コードおよびファイルの文字エンコードを統一します。

ethertank commented 11 years ago

CSS ファイルがどの改行コードを使用しているか分からず、

やや分かりにくかったかもしれませんが、 CR LF だったと私が言っているのは style.css と rtl.css についてです。 おっしゃる通り、CSS 以外のファイルも混在しているようですね。

ご提示いただいたソフトは確認用ではなく書き換え用のソフトの様ですが、これを使って変換後、コミットして確認せよとの事でしょうか?

tetsuharuohzeki commented 11 years ago

ご提示いただいたソフトは確認用ではなく書き換え用のソフトの様ですが、これを使って変換後、コミットして確認せよとの事でしょうか?

いえ、統一すべきファイルがこれで大丈夫なのかについて、現状の確認だけお願いします。 改行コードと文字コードの変更については、私の方で https://github.com/mozilla-japan/modest_themes/pull/42 で一旦Pull Requestを出しました <( )>

ethertank commented 11 years ago

おっしゃる意味が理解できなかったので、ご紹介いただいたソフトは用いずに改行コードについて確認しました。 master の、以下のファイルが改行コードに CR LF を用いているようです。

tetsuharuohzeki commented 11 years ago

返信遅れてすみません。

おっしゃる意味が理解できなかったので

一部のエディタで、英数字しか使っていないBOM無しのutf-8文字コードのファイルの文字エンコードや改行コードを誤認するバグなどが存在しているため、確実性を重視した為、そのソフトを紹介しました。言葉足らずですみませんでした。

さて、長丁場に成ってしまった本件の解決について、私が現在考えているフローとしては、

  1. @ethertank さんが一番最初にプルリクエストしたコードの書式整形に関するものだけをマージ
  2. 1.の後、文字コード・改行コードの統一について、それぞれ一つのコミットに集約したものをマージ

の順番で、この issue を無事 close できるのではないかと思っています。

@ethertank さんは、どう思われますか?

ethertank commented 11 years ago

先に @saneyuki さんに #42 で統一していただいて先にマージ。こちらも全て統一から再コミット。問題が無ければマージ…という形になるのかな…と考えていました。そうすれば、このコミットの差分も確認しやすいのではないかと…。

お任せします。

tetsuharuohzeki commented 11 years ago

わかりました。先に #42 をマージします。

tetsuharuohzeki commented 11 years ago

CLOSED: https://github.com/mozilla-japan/modest_themes/pull/42

それでは、お願いします。

ethertank commented 11 years ago

全て統一したつもりですが… page.php 、404.php 、 それと css の 3 ファイルの差分がおかしいですね…。

ethertank commented 11 years ago

お手数ですが、現在の master に Shift-JIS のものが混入していないかご確認いただけないでしょうか? こちらで確認する限りでは、 admin/admin.css などの幾つかのファイルが Shift-JIS になっているような気がします。

私の間違いでしたら申し訳ないですが、ご協力宜しくお願いします。

tetsuharuohzeki commented 11 years ago

nkfコマンドで確認した所、確かに一部のファイルがascii扱いになってますね……

utf-8で保存していますが、ファイル内容がasciiコードのみで出力されている為の誤検知かもしれません。

ethertank commented 11 years ago

ローカルで確認する限り、かかる変更による問題は発生していないように思います。

おそらく誤検知ではなく、実際に Shift-JIS になっています。 差分が確認しにくいですが、このリクエストを通していただければ混在の問題の残りは解決すると思います。 もしよろしければこのまま通していただく思います。

それでは問題があるという事でしたら、先んじて残りのものの変換をお願いします。

tetsuharuohzeki commented 11 years ago

おそらく誤検知ではなく、実際に Shift-JIS になっています

shift-jisもutf-8も、英数字部分はascii形式の文字コードの集合です。プラットフォーム・エディタによってデフォルトの文字コードが違う為、shift-jisとして解釈されているのだと思います。

pull requestですが、conflictが発生しているのでこのままではマージが出来ません。rebaseまたはマージによる競合の解決をお願いします。

tetsuharuohzeki commented 11 years ago

確認おくれてすみません。 うーん、、、まだ自動マージが可能な状態ではないみたいです……