-
Notifications
You must be signed in to change notification settings - Fork 11
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
More client features #157
More client features #157
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm approving this. Obviously it's a lot of code, mostly scaffolding, and hard to review in a thorough sense. I've looked through, and learned a few things, and it all looks great and carefully done. I don't think there's a need at this early stage of the project to block merging this on careful review by others.
.github/workflows/ci.yml
Outdated
working-directory: ./temporalio | ||
run: | | ||
bundle exec rake proto:generate | ||
[[ -z $(git status --porcelain lib/temporalio/api) ]] || (git diff lib/temporalio/api; echo "Protos changed"; exit 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[[ -z $(git status --porcelain lib/temporalio/api) ]] || (git diff lib/temporalio/api; echo "Protos changed"; exit 1) | |
[[ -z $(git status --porcelain lib/temporalio/api) ]] || (git diff lib/temporalio/api; echo "Protos changed" 1>&2; exit 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was taken from https://github.com/temporalio/sdk-python/blob/4aef4bfda19e56c7e7abac36b69e98f60861468c/.github/workflows/ci.yml#L95 so I wonder if we want to fix there too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep sounds good. Not urgent of course; could do it now or just do it next time one of us touches the CI yaml.
What was changed