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

Cleanup readme #48

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

greenie-msft
Copy link

I made some updates to the README. Here's a summary of the changes:

  • Main topics have been moved to their own separate documents outside of the main README.
  • Legacy content has been removed
  • The Getting Started section now uses one of the DTS quickstarts hosted in the repository.

docs/features.md Outdated

Orchestrations can be suspended using the `suspend_orchestration` client API and will remain suspended until resumed using the `resume_orchestration` client API. A suspended orchestration will stop processing new events, but will continue to buffer any that happen to arrive until resumed, ensuring that no data is lost. An orchestration can also be terminated using the `terminate_orchestration` client API. Terminated orchestrations will stop processing new events and will discard any buffered events.

### Retry policies (TODO)
Copy link
Member

Choose a reason for hiding this comment

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

This (TODO) can be removed

docs/features.md Outdated

Orchestrations can wait for external events using the `wait_for_external_event` API. External events are useful for implementing human interaction patterns, such as waiting for a user to approve an order before continuing.

### Continue-as-new (TODO)
Copy link
Member

Choose a reason for hiding this comment

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

This (TODO) can be removed

@@ -0,0 +1,13 @@
# Contributing
Copy link
Member

Choose a reason for hiding this comment

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

A CONTRIBUTING.md (all caps) file is typically expected to exist at the root of a repo.


As an aside, you'll also notice that the example orchestration above works with custom business objects. Support for custom business objects includes support for custom classes, custom data classes, and named tuples. Serialization and deserialization of these objects is handled automatically by the SDK.

You can find the full sample [here](./examples/human_interaction.py).
Copy link
Member

Choose a reason for hiding this comment

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

Do all the links to samples need to be fixed now that you've changed the location of this content?

Suggested change
You can find the full sample [here](./examples/human_interaction.py).
You can find the full sample [here](../examples/human_interaction.py).

@@ -0,0 +1,102 @@
# Portable SDK Sample for Sub Orchestrations and Fan-out / Fan-in
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this go directly under examples? Also, if we're updating the focus of the repo to just be DTS, then we don't need a dts subfolder under examples. All samples can assume DTS and have corresponding instructions for using the emulator.


Create a Scheduler:
```bash
az durabletask scheduler create --resource-group --name --location --ip-allowlist "[0.0.0.0/0]" --sku-capacity 1 --sku-name "Dedicated" --tags "{'myattribute':'myvalue'}"
Copy link
Member

Choose a reason for hiding this comment

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

I suggest we guide readers to use the emulator rather than creating resources in Azure. That will save them a ton of time and effort when they're still learning. Let's have separate instructions and/or more substantial samples for configuring apps with DTS in Azure.

docker run --name dtsemulator -d -p 8080:8080 mcr.microsoft.com/dts/dts-emulator:v0.0.4
```

3. Set the Environment Variables:
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove environment variables from the local emulator setup instructions. The sample code can assume localhost and default if these variables don't exist. This will save users a lot of time and effort when getting started.

if taskhub_name:
print(f"The value of TASKHUB is: {taskhub_name}")
else:
print("TASKHUB is not set. Please set the TASKHUB environment variable to the name of the taskhub you wish to use")
Copy link
Member

Choose a reason for hiding this comment

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

Again, I suggest we remove this requirement. We can assume default values for emulator usage if these environment variables aren't sent.

if taskhub_name:
print(f"The value of TASKHUB is: {taskhub_name}")
else:
print("TASKHUB is not set. Please set the TASKHUB environment variable to the name of the taskhub you wish to use")
Copy link
Member

Choose a reason for hiding this comment

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

Same comment about default values for these environment variables.

- [Available Features](./docs/features.md)
- [Getting Started](./docs/getting-started.md)
- [Development Guide](./docs/development.md)
- [Contributing Guide](./docs/development.md)

## Trademarks
Copy link
Member

Choose a reason for hiding this comment

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

nit: I see you're removing whitespace between titles and content, but MS Learn markdown conventions require an extra newline between section titles and content. I suggest we do the same here.

Suggested VS Code extension: https://marketplace.visualstudio.com/items?itemName=DavidAnson.vscode-markdownlint

@cgillum cgillum self-requested a review April 10, 2025 22:26
@cgillum
Copy link
Member

cgillum commented Apr 10, 2025

Looks like the removal of the dts directory caused a CI issue, so we probably need to also update the CI yaml contents.

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.

None yet

2 participants