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

Add section headings to projections HTTP API docs #4101

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hayley-jean
Copy link
Member

The HTTP API docs were missing headings for each operation, which makes it difficult to find what you're looking for

### Create a Continuous Projection

| URI | Description | HTTP verb |
|:---------------------------------------------------|:-----------------------------|:----------|
| `/projections/continuous?name={name}&type={type}&enabled={enabled}&emit={emit}&trackemittedstreams={trackemittedstreams}` | Create a continuous projection. This type of projection will, if enabled will continuously run unless disabled or an unrecoverable error is encountered. | POST |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| `/projections/continuous?name={name}&type={type}&enabled={enabled}&emit={emit}&trackemittedstreams={trackemittedstreams}` | Create a continuous projection. This type of projection will, if enabled will continuously run unless disabled or an unrecoverable error is encountered. | POST |
| `/projections/continuous?name={name}&type={type}&enabled={enabled}&emit={emit}&trackemittedstreams={trackemittedstreams}` | Create a continuous projection. This type of projection will, if enabled, continuously run unless disabled or an unrecoverable error is encountered. | POST |

@@ -209,6 +221,8 @@ The server then returns the state for the partition:
* `emit`: Allow the projection to write to streams (true/false)
* `trackemittedstreams`: Write the name of the streams the projection is managing to a separate stream. $projections-{projection-name}-emittedstreams (true/false)

### Delete a Projection

| URI | Description | HTTP verb |
|:-------------------------------------------------------------------------------------------------------------------------------------------------------|:------------------------------------------------------------------------------------------------|:----------|
| `/projection/{name}` | Returns information for a projection. | GET |
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the verb be delete?

Suggested change
| `/projection/{name}` | Returns information for a projection. | GET |
| `/projection/{name}` | Returns information for a projection. | DELETE |

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah there were actually two different actions in the table. I think the original formatting was meant to be closer to what's in the most recent commit. It's up to you whether that's more readable than different sections per action @cdevarenne @rsowald 🙂

@@ -221,6 +235,8 @@ The server then returns the state for the partition:
* `deleteCheckpointStream`: Delete the checkpoint stream (true/false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the arguments be shown in the example?

Copy link
Contributor

Choose a reason for hiding this comment

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

from the other examples, it looks like just including it in the URI is setting it true. Omitting is false. This is unclear in the documentation though. If that's the case, I think we should just remove the (true/false) from the descriptions. So, it's also unclear if any/all of these parameters are required.

Copy link
Member Author

Choose a reason for hiding this comment

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

it looks like just including it in the URI is setting it true. Omitting is false.

@rsowald This isn't always the case. Omitting it is leaving it as the default (which we should list in the parameters), but if you want to set it you should use either deleteCheckpointStream=true or deleteCheckpointStream=false

Should the arguments be shown in the example?

@cdevarenne Do you mean something like this?

/projections/continuous?name={name}&type={ js | native }&enabled={ true | false }&emit={ true | false }&trackemittedstreams={ true | false }

@hayley-jean hayley-jean force-pushed the hayley-jean/update-projections-http branch from b327a81 to d1ebd2e Compare December 19, 2023 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants