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

Support to run local game.json #23

Merged
merged 10 commits into from
May 16, 2019
Merged

Support to run local game.json #23

merged 10 commits into from
May 16, 2019

Conversation

yu-ogi
Copy link
Contributor

@yu-ogi yu-ogi commented May 13, 2019

このPullRequestが解決する内容

  • ローカルディレクトリの game.json から Runner を起動可能に
  • テストが肥大化してきたので分割

@@ -0,0 +1,157 @@
import { setSystemLogger } from "../Logger";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

こちらはテスト分割による差分です。
テスト内容に変更はありません。

@@ -0,0 +1,449 @@
import { GetStartPointOptions, StartPoint } from "@akashic/amflow";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

こちらはテスト分割による差分です。
テスト内容に変更はありません。

@@ -863,6 +215,73 @@ describe("コンテンツ動作テスト", () => {
});
});

describe("ローカルコンテンツの動作テスト", () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

こちらが本修正によるテストになります。

@yu-ogi yu-ogi requested review from dera- and xnv May 14, 2019 01:05
return await this.loadJSON(contentUrl);
}

protected async resolveGameConfiguration<T>(gameJsonUrl: string): Promise<T> {
Copy link
Member

Choose a reason for hiding this comment

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

型にバリエーションがあるような名前に見えませんが、この T は必要なんでしょうか……?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a3114f8
こちら generics を利用しないように修正しました。


export interface PlayWithContentDir {
/**
* game.json を含むディレクトリ。
Copy link
Member

Choose a reason for hiding this comment

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

「game.json を含むディレクトリ」を指定することになっていますが、game.json のパスを指定するようにはできないでしょうか。というのも、「ローカルディレクトリか URL か」と「game.json か content.json か」は独立だと思えるからです。headless-driver は概念的には「content.json を含むディレクトリ」や「game.json の URL」も扱えていいと思います。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

こちら少し迷った部分だったのですが、おっしゃる通り game.json のローカルパスを指定する gameJsonPath に変更しました。

  • ローカルの content.json を扱う ( contentPath )
  • URL上の game.json を扱う ( gameJsonUrl )

については別 PullRequest での対応とさせてください。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ホスティング時のテスト部分で結構無理をしているため、先にそちらの改修を行いたいためです。

protected async resolveContent(contentUrl: string): Promise<any> {
const config = await loadFile<any>(contentUrl, { json: true });
if (config.content_url === "v1_content_url") {
config.content_url = gameJsonUrlV1;
} else if (config.content_url === "v2_content_url") {
config.content_url = gameJsonUrlV2;
} else if (config.content_url === "v2_content_cascade_url") {
config.content_url = cascadeGameJsonUrlV2;
}
if (config.asset_base_url === "v1_asset_base_url") {
config.asset_base_url = assetBaseUrlV1;
} else if (config.asset_base_url === "v2_asset_base_url") {
config.asset_base_url = assetBaseUrlV2;
}
return config;
}

Copy link
Member

@xnv xnv left a comment

Choose a reason for hiding this comment

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

コメントつけましたが approved.

export type Play = (PlayWithContentUrl | PlayWithContentDir) & BasePlay;
export type Play = PlayLocation & BasePlay;

export type PlayLocation = (PlayWithContentUrl | PlayWithGameJsonPath);
Copy link
Member

Choose a reason for hiding this comment

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

play に場所はなさそうに思え、これはむしろ ContentLocation とでも呼ぶ方が実情に近いのではないでしょうか。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

おっしゃる通りでしたので修正しました。
a807d86

@yu-ogi yu-ogi merged commit 5a063b3 into master May 16, 2019
@yu-ogi yu-ogi deleted the support-local-game.json branch May 16, 2019 00:56
@yu-ogi yu-ogi mentioned this pull request May 16, 2019
@yu-ogi yu-ogi added the enhancement New feature or request label May 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants