Skip to content

Update webhook docs - #1432

Open
JunAishima wants to merge 16 commits into
bluesky:mainfrom
JunAishima:update-webhook-docs
Open

Update webhook docs#1432
JunAishima wants to merge 16 commits into
bluesky:mainfrom
JunAishima:update-webhook-docs

Conversation

@JunAishima

Copy link
Copy Markdown
Contributor

Checklist

  • Add a Changelog entry
  • Add the ticket number which this PR closes to the comment section

@JunAishima

Copy link
Copy Markdown
Contributor Author

#1431

@danielballan danielballan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for taking a look at this.

As I look over my line review here, I think we brought different expectations to this document. I want it to guide the user down a prescribed path to a minimal working example. I want to avoid presenting them with choices or applied examples that might pull in a lot of additional context ("RunEngine").

I'm also wondering there is some confusion around whether it works with vanilla python or requires IPython. I think this will work fine with vanilla python as is. I'm inclined to close the PR unless we can confirm an issue there.

This tutorial demonstrates the full webhook lifecycle end-to-end in a single
Python session, with no external services and no configuration files required.
IPython session, with no external services and no configuration files required.
Using IPython is necessary to instantiate and persist the classes and services.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I remember you mentioning this, but I question it. What "persistence" is IPython providing exactly? Can you provide a minimal example of what fails if you use a plain python interpreter?

If this doesn't working with a vanilla python interpreter, I think I would rather update the example than require IPython to run it.

IPython session, with no external services and no configuration files required.
Using IPython is necessary to instantiate and persist the classes and services.

The server and receiver can be made more modular by using the port and API key information

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A guiding principle of diataxis tutorials is: don't present the user with choices. They don't have enough context to make choices yet. Just prescribe. I don't think we're at the point yet where we need to be reconfiguring...let's stick to a single process.

print(f"Receiver listening on http://127.0.0.1:{receiver_port}/hook")
```

Note that the `received` variable will be accessible from the IPython session,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this is helping. All variables are accessible from whatever Python interpreter you are using...

"Authorization": f"Apikey {server.api_key}",
"Content-Type": "application/json",
},
content=json.dumps({"url": f"http://127.0.0.1:{receiver_port}/hook"}),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I find the simpler version (without explicit Nones) more appropriate style for a tutorial.

```

## See also
# Extending toward real-world situations

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Recapping a Slack conversation from a week or so ago: We studiously keep "bluesky" separate here to make clear that tiled is useful on generic data, not just bluesky data. Let's wait to add this as part of a larger section of "bluesky integration." We don't have space in this document to contextualize "RunEngine," "stop document," and so on for Tiled users.

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