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

docs(server): new docs site for ARFlow server #2

Merged
merged 7 commits into from
Sep 4, 2024

Conversation

FelixNgFender
Copy link
Contributor

I replaced the static.yml CI action and the bash scripts used to build & copy the docs to the static website. We now run all of that on a GitHub runner with a new website.yml action.

Added docs for Protobuf schema, 1 action item left here though: the old message field in RegisterResponse has been changed in the Protobuf schema, but the change has not been reflected in the generated data access layer for the C# client.

message RegisterResponse {
    string uid = 1; // previously named `message`
}

Centralized all remaining bash scripts in python/ to Poetry scripts inside pyproject.toml.

Added docs for important packages & tools used in the server.

@YiqinZhao YiqinZhao self-requested a review August 30, 2024 13:03
@YiqinZhao
Copy link
Member

Thanks for the contribution, a few thoughts of mine:

  • Why is the message field changed to uid? To align with DataFrameRequest?
  • I don't think centralizing all bash script code to the pyproject.toml file is a good practice. The Python side code will be published to pypi as both a package and a CLI tool. The command defined in the pyproject.toml file will be exposed as CLI subcommand, for example: arflow serve. The scripts for executing example code shouldn't be added to there.

Copy link
Member

@YiqinZhao YiqinZhao left a comment

Choose a reason for hiding this comment

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

Added more comments

protos/arflow/service.proto Show resolved Hide resolved
python/pyproject.toml Outdated Show resolved Hide resolved
python/pyproject.toml Outdated Show resolved Hide resolved
@YiqinZhao
Copy link
Member

The changes look good now. Will merging this PR break the server? Because the uid field updates are not reflected on the Unity side.

@YiqinZhao YiqinZhao added WPI Projects created by WPI students. MQP Project related to WPI MQP projects. labels Sep 1, 2024
Copy link
Member

@YiqinZhao YiqinZhao left a comment

Choose a reason for hiding this comment

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

LGTM

@YiqinZhao YiqinZhao merged commit 7c13069 into cake-lab:main Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
MQP Project related to WPI MQP projects. WPI Projects created by WPI students.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants