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

kana-rule.confでk;kqのようなセミコロン/qを含むルールを許可 #120

Merged
merged 5 commits into from
Feb 22, 2024

Conversation

y-yu
Copy link
Contributor

@y-yu y-yu commented Feb 20, 2024

  • スペースを含むルールについては ローマ字変換ルールを設定ファイル化 #96 で対応が行なわれていたが;にはこのような特殊処理がはいっていなかった
  • ACTのような拡張では、日本語入力で利用頻度の低い;qのようなキーに特殊な役割を与えることがあり、このために必要となった
  • これのテストを行うにあたってStateMachineクラスでグローバルに定義されたkanaRuleを参照しているとデフォルトのkana-rule.confが利用されてしまうため、テスト目的でStateMachineのコンストラクターでkanaRule: Romaji!を受けとるようにした
    • 省略すれば従来どおりグローバルに定義されたkanaRuleを用いる

@y-yu
Copy link
Contributor Author

y-yu commented Feb 20, 2024

🤔

Run kishikawakatsumi/xcresulttool@v1
Error: Resource not accessible by integration

@y-yu y-yu changed the title kana-rule.confでk;のようなセミコロンを含むルールを許可 kana-rule.confでk;kqのようなセミコロン/qを含むルールを許可 Feb 21, 2024
@mtgto
Copy link
Owner

mtgto commented Feb 21, 2024

Pull Requestありがとうございます。

Error: Resource not accessible by integration

おそらく作者自身のPull Requestではない、Fork先からのPull Requestだからなんらかの権限が足りないのだろうと思います。
ちょっと見てみます。

@mtgto
Copy link
Owner

mtgto commented Feb 21, 2024

https://docs.github.com/ja/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token

によると、GITHUB_TOKENについて 既定のアクセス (制限あり) のところだと、checksについてはなしになっているのでそれなのかなあ。

kishikawakatsumi/xcresulttool@v1 で検索すると https://github.com/mlemgroup/mlem とか permissions: write-all することでforkからのプルリクエストでも実行できてそう。

ためしに checks: write を付与してみます。

@mtgto
Copy link
Owner

mtgto commented Feb 21, 2024

うーん、Re-Runだと古いWorkflowで実行されてしまって意味がなかった。

Copy link
Owner

@mtgto mtgto left a comment

Choose a reason for hiding this comment

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

ありがとうございます。私は z + spaceで全角スペースを登録していたのでそこだけ対応してましたが、対応漏れがあったのに気付いていませんでした。
細かいスタイルの修正だけお願いします。

macSKK/StateMachine.swift Outdated Show resolved Hide resolved
@@ -1063,6 +1063,47 @@ final class StateMachineTests: XCTestCase {
XCTAssertTrue(stateMachine.handle(printableKeyEventAction(character: "u", withShift: true)))
wait(for: [expectation], timeout: 1.0)
}

func testHandleComposingSpaceAfterPrintable() {
let stateMachine = StateMachine(initialState: IMEState(inputMode: .hiragana), kanaRule: try! Romaji(source: "z ,スペース"))
Copy link
Owner

Choose a reason for hiding this comment

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

雑談: ちなみにdictionaryというグローバル変数をStateMachineでも参照しています。StateMachineTestsではコンストラクタに渡すDI方式ではなく、グローバル変数dictionaryをテストの中で直接書き換えています。
このPull RequestとしてはコンストラクタでkanaRuleを渡す形式のままでオーケーです。なんかしっくりこないなと思ったらグローバル変数を直接テストでかきかえるように変更しちゃうかもしれないですし、dictionaryもStateMachineのコンストラクタに渡すように変えるかもしれません。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

なるほど、たしかにその方法でもよかったですね 😇

@mtgto
Copy link
Owner

mtgto commented Feb 21, 2024

CIが落ちるのだけ実験したいので、試しにmainブランチをmergeしてみます。

@mtgto
Copy link
Owner

mtgto commented Feb 21, 2024

変わらず Error: Resource not accessible by integration で失敗。うーん。

@mtgto
Copy link
Owner

mtgto commented Feb 21, 2024

https://til.toshimaru.net/2021-12-12
pull_requestではなく、pull_request_targetをイベントのトリガーにすることでforkedリポジトリからのPull Requestでの実行でも書き込み権限を与えることができる。
ただしSecretsもアクセスできてしまうので危険なコードが実行されないように気をつけないといけない。

またpull_request_targetをイベントのトリガーとすると、なにも考えずに actions/checkout でcheckoutするとマージしたときのリビジョンではなく、base refがcheckoutされてしまう。これは古いリビジョンでテストが実行されてしまうためあまりうれしくはない。
github apiでmerged revisionを取得することはできるみたい https://zenn.dev/shunsuke_suzuki/articles/secure-github-actions-by-pull-request-target#merge-commit-%E3%81%AE-checkout

https://securitylab.github.com/research/github-actions-preventing-pwn-requests/
にあるように、pull_requestイベントではテスト結果をartifactとしてアップロードしておき、workflow_run (default branth) でartifactをダウンロードして書き込みも行うという方法もある。
ただそれには kishikawakatsumi/xcresulttool が対応してくれないとだめな気がする。
いまはchecksの更新とartifactのアップロードをいっしょくたにやっているため、分離できないと思われる。
https://github.com/kishikawakatsumi/xcresulttool/blob/v1.7.1/src/main.ts#L104-L145
(artifactのアップロードはinputで制御できるけどchecksの作成は制御できない。自分はartifactはほしいけどchecksは別になくてもいいんだけどなあ)

とりあえずpull_request_targetにしておき、merged revisionをcheckoutして実行するようにするかなあ。
並行してkishikawakatsumi/xcresulttoolに「checksの作成だけ無効にできるオプションの追加」を提案してもいいかもしれない。

@mtgto
Copy link
Owner

mtgto commented Feb 21, 2024

とりあえずpull_request_targetにしておき、merged revisionをcheckoutして実行するようにするかなあ。

#121 にちょこっと書きましたが、pull_request_targetを対象とすることでCIが全部動くようになりました。
上のコメントではmerged revisionでチェックアウトしようと書いたんですが、pull_request_targetイベントはマージ可能じゃなくても動くのもあったりして、merged revisionをcheckoutするのはめんどうになってやめました。

@mtgto
Copy link
Owner

mtgto commented Feb 22, 2024

ありがとうございました。今週末にリリースバイナリを作ろうと思います。

@mtgto mtgto merged commit 839cabc into mtgto:main Feb 22, 2024
2 checks passed
@y-yu y-yu deleted the semicolon-as-printable-if-romaji-is-not-empty branch February 22, 2024 11:54
@y-yu
Copy link
Contributor Author

y-yu commented Feb 22, 2024

ありがとうございます!

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.

2 participants