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

整理: ライセンス生成の重複処理を共通化 #1153

Closed
wants to merge 7 commits into from

Conversation

tarepan
Copy link
Contributor

@tarepan tarepan commented Mar 29, 2024

内容

概要: ライセンス生成の重複処理を共通化してリファクタリングした

従来の generate_licenses.py ではライセンス情報の生成時に「ライセンス文アクセス→License インスタンス化→licenses 登録」を約20個の個別ライセンスごとにベタ書きしており、一切の共通化がなされていなかった。
これら重複処理を for 文により共通化した。
すなわち、以前の

   licenses.append(
        License(
            name="Open JTalk",
            version="1.11",
            license="Modified BSD license",
            text=Path("docs/licenses/open_jtalk/COPYING").read_text(),
        )
    )
    licenses.append(
        License(
            name="MeCab",
            version=None,
            license="Modified BSD license",
            text=Path("docs/licenses/open_jtalk/mecab/COPYING").read_text(),
        )
    )
    with urllib.request.urlopen(
        "https://raw.githubusercontent.com/VOICEVOX/voicevox_core/main/LICENSE"
    ) as res:
        licenses.append(
            License(
                name="VOICEVOX CORE",
                version=None,
                license="MIT license",
                text=res.read().decode(),
            )
        )

を次のように共通化した:

    license_infos: list[LicenseInfo] = [
        ("local",   "Open JTalk",    "1.11", "Modified BSD license", "docs/licenses/open_jtalk/COPYING"),
        ("local",   "MeCab",         None,   "Modified BSD license", "docs/licenses/open_jtalk/mecab/COPYING"),
        ("remote",  "VOICEVOX CORE", None,   "MIT license",          "https://raw.githubusercontent.com/VOICEVOX/voicevox_core/main/LICENSE"),
    ]

    for info in license_infos:
        if info[0] == "local":
            licenses.append(
                License(
                    name=info[1],
                    version=info[2],
                    license=info[3],
                    text=Path(info[4]).read_text(),
                )
            )
        elif info[0] == "remote":
            with urllib.request.urlopen(info[4]) as res:
                licenses.append(
                    License(
                        name=info[1],
                        version=info[2],
                        license=info[3],
                        text=res.read().decode(),
                    )
                )

license_infos の可読性を上げるためにこの箇所における linter をオフにし、space 挿入による整形をおこなっている。
折り返し無効化により GitHub PR UI 上での可読性が下がっているため、エディタでチェックして頂ければ幸いです。

関連 Issue

無し

Notes

python build_util/generate_licenses.py -o ./opt.json の出力がリファクタリング前後で完全一致することを確認済み

@tarepan tarepan requested a review from a team as a code owner March 29, 2024 07:13
@tarepan tarepan requested review from y-chan and removed request for a team March 29, 2024 07:13
Copy link

Coverage Result

Resultを開く
Name Stmts Miss Cover
run.py 517 210 coverage-59%
voicevox_engine/init.py 1 0 coverage-100%
voicevox_engine/cancellable_engine.py 94 72 coverage-23%
voicevox_engine/core/init.py 0 0 coverage-100%
voicevox_engine/core/core_adapter.py 81 6 coverage-93%
voicevox_engine/core/core_initializer.py 60 30 coverage-50%
voicevox_engine/core/core_wrapper.py 225 157 coverage-30%
voicevox_engine/dev/init.py 0 0 coverage-100%
voicevox_engine/dev/core/init.py 0 0 coverage-100%
voicevox_engine/dev/core/mock.py 65 2 coverage-97%
voicevox_engine/dev/tts_engine/init.py 0 0 coverage-100%
voicevox_engine/dev/tts_engine/mock.py 28 0 coverage-100%
voicevox_engine/engine_manifest/EngineManifest.py 36 0 coverage-100%
voicevox_engine/engine_manifest/EngineManifestLoader.py 12 0 coverage-100%
voicevox_engine/engine_manifest/init.py 0 0 coverage-100%
voicevox_engine/library_manager.py 92 4 coverage-96%
voicevox_engine/metas/Metas.py 36 0 coverage-100%
voicevox_engine/metas/MetasStore.py 28 1 coverage-96%
voicevox_engine/metas/init.py 0 0 coverage-100%
voicevox_engine/model.py 180 3 coverage-98%
voicevox_engine/morphing.py 71 4 coverage-94%
voicevox_engine/preset/Preset.py 13 0 coverage-100%
voicevox_engine/preset/PresetError.py 2 0 coverage-100%
voicevox_engine/preset/PresetManager.py 80 2 coverage-98%
voicevox_engine/preset/init.py 0 0 coverage-100%
voicevox_engine/setting/Setting.py 9 0 coverage-100%
voicevox_engine/setting/SettingLoader.py 20 0 coverage-100%
voicevox_engine/setting/init.py 0 0 coverage-100%
voicevox_engine/tts_pipeline/init.py 0 0 coverage-100%
voicevox_engine/tts_pipeline/kana_converter.py 88 1 coverage-99%
voicevox_engine/tts_pipeline/mora_mapping.py 7 0 coverage-100%
voicevox_engine/tts_pipeline/phoneme.py 34 0 coverage-100%
voicevox_engine/tts_pipeline/text_analyzer.py 146 6 coverage-96%
voicevox_engine/tts_pipeline/tts_engine.py 267 9 coverage-97%
voicevox_engine/user_dict/part_of_speech_data.py 5 0 coverage-100%
voicevox_engine/user_dict/user_dict.py 146 12 coverage-92%
voicevox_engine/utility/init.py 0 0 coverage-100%
voicevox_engine/utility/connect_base64_waves.py 37 0 coverage-100%
voicevox_engine/utility/core_utility.py 6 0 coverage-100%
voicevox_engine/utility/core_version_utility.py 8 1 coverage-88%
voicevox_engine/utility/mutex_utility.py 13 0 coverage-100%
voicevox_engine/utility/path_utility.py 26 6 coverage-77%
voicevox_engine/utility/run_utility.py 10 7 coverage-30%
TOTAL 2443 533 coverage-78%

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

見やすい反面、他の貢献者の方の妨げになっちゃうかもとちょっと思いました。
結構意見分かれる気がするので遠慮なく意見いただけると!!

build_util/generate_licenses.py Show resolved Hide resolved
build_util/generate_licenses.py Show resolved Hide resolved
build_util/generate_licenses.py Show resolved Hide resolved
@tarepan
Copy link
Contributor Author

tarepan commented Apr 7, 2024

#1153 (comment) のファクトリに着想を得た alternative 実装として #1177 を提案しました。

@Hiroshiba
本 PR と #1177 を比較した所感をお聞きできればと思います。

@tarepan tarepan closed this Apr 14, 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