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

Feat/dearchive #30

Merged
merged 13 commits into from
Jun 7, 2021
Merged

Feat/dearchive #30

merged 13 commits into from
Jun 7, 2021

Conversation

emadurandal
Copy link
Contributor

@emadurandal emadurandal commented Jun 1, 2021

すみません。立て続けにPRを送ってしまいます💦

archiveスクリプトいいですね。早速使ってます!

ただ、対応するdearchiveを自分で毎回手作業コマンドというのも面倒だったので、自前で勝手に書いてしかもせっかく作ったしなにかの足しになればと勇み足でのPRです💦💦

おそらく @KEINOS @yoshi389111 お二人の方で似たコマンドを計画されていたと思うので、しゃしゃり出てしまう感じで大変恐縮です。あくまで一つの案ですので、部分だけ使えるコードがあれば部分だけ取り込むとかでも、却下~でもかまいません笑

第4引数のは自分なりにあれば便利だな、と思って付けた感じです。
(archiveコマンドにも <github user> をさらにオプションでつけたら共通鍵を暗号化してくれるおまけをつけると素敵かも)

@emadurandal
Copy link
Contributor Author

.github/run-lint.sh の LIST_SCRIPT_NO_EXTにはまだ追加していません( #27 (comment) の方とコンフリクトしそうなので)

Copy link
Collaborator

@yoshi389111 yoshi389111 left a comment

Choose a reason for hiding this comment

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

気になる点をいくつか挙げます。

すみませんが、ご確認ください。


追記:

指摘がかぶりましたね。すみません。

また、私の方では dearchive に相当するものを作ってはいなかったので、PR 助かります 👍

bin/dearchive Outdated Show resolved Hide resolved
bin/dearchive Outdated Show resolved Hide resolved
bin/dearchive Outdated Show resolved Hide resolved
KEINOS
KEINOS previously requested changes Jun 1, 2021
Copy link
Member

@KEINOS KEINOS left a comment

Choose a reason for hiding this comment

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

実は archive コマンドは、enc コマンドでファイルが大きかった場合に内部で呼び出して使おうと思っていたコマンドです。(ファイルが大きくても小さくても enc 1 つで済むように)

そうなると「 dec コマンドも解凍→復号処理必要だよなぁ」と思って放置していました ... あざっす!

以下チェック内容です。LIST_SCRIPT_NO_EXT への追記も忘れずにお願いしまーす。

bin/dearchive Outdated Show resolved Hide resolved
bin/dearchive Outdated Show resolved Hide resolved
bin/dearchive Outdated Show resolved Hide resolved
bin/dearchive Outdated Show resolved Hide resolved
@KEINOS
Copy link
Member

KEINOS commented Jun 1, 2021

おおっと!かぶってしまったなりよー!wwww
おいどんの修正リクは取り下げるなりね!

@KEINOS KEINOS dismissed their stale review June 1, 2021 12:48

パンを咥えた @yoshi389111 さんとぶつかったため

@emadurandal
Copy link
Contributor Author

@yoshi389111 @KEINOS ご指摘ありがとうございます!
シェルスクリプトにも書式的なlintがあるのですね。頑張ってなれるようにします。
明日以降やっていければ。

@KEINOS
Copy link
Member

KEINOS commented Jun 3, 2021

@emadurandal

ローカル環境を色々いじる(コマンドを色々入れる)のが大変であれは、とりあえず LIST_SCRIPT_NO_EXTdearchive コマンド追加してコミットを push してみてください。

GitHub 上の Linter と UnitTest が働き、おそらくテストが失敗するので、そのエラー内容から 1 つずつ潰して行った(修正→コミット→チェックのループにした)方がやりやすいと思います。

コミットが細々となりますが、mainmaster にマージするときには Squash & Merge で 1 つのコミットにまとめちゃうので、大丈夫!ノープロ! 👍

@emadurandal
Copy link
Contributor Author

@yoshi389111 @KEINOS もろもろ修正しました。ご確認よろしくおねがいします。

Copy link
Member

@KEINOS KEINOS left a comment

Choose a reason for hiding this comment

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

echo -n は Issue #43 で、$RANDOM の POSIX 対応は #49 で一緒に直すので、それ以外の 2 点の修正をお願いしまーす。

bin/dearchive Outdated Show resolved Hide resolved
bin/dearchive Outdated Show resolved Hide resolved
@emadurandal
Copy link
Contributor Author

@KEINOS 修正しましたー

Copy link
Member

@KEINOS KEINOS left a comment

Choose a reason for hiding this comment

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

あらまー、修正されてるわ。私的には LGTM でございます。

obama_drunk

@yoshi389111 先生的にはいかがですか?

bin/dearchive Outdated Show resolved Hide resolved
@emadurandal
Copy link
Contributor Author

@yoshi389111 @KEINOS おっと、ご指摘ありがとうございます。+なくてもいいんですね。ここらへん慣れていきたい。

@yoshi389111
Copy link
Collaborator

ありがとうございます。問題ないと思います

LGTM

@KEINOS KEINOS merged commit 38b1760 into Qithub-BOT:master Jun 7, 2021
@KEINOS
Copy link
Member

KEINOS commented Jun 7, 2021

マージし master!! 🎉

@emadurandal emadurandal deleted the feat/dearchive branch June 7, 2021 02:52
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