Skip to content

Conversation

mnocon
Copy link
Contributor

@mnocon mnocon commented Feb 6, 2025

Doc for endpoints introduced in:

Preview

Questions:

  1. The GET /ai/actions and POST /ai/actions return the same data - I wasn't sure how to name these entries, I've chosen:
  • List action configurations
  • List action configurations (POST)

but maybe there's something better

@github-actions
Copy link

Preview of modified Markdown: no Markdown change to preview.

@mnocon
Copy link
Contributor Author

mnocon commented Feb 13, 2025

@barw4 thank you for the review, I've applied the types changes in #2605 and the ActionConfiguration responses are regenerated in 4864e0d

@mnocon mnocon requested a review from barw4 February 13, 2025 15:04
Copy link
Contributor

@dabrt dabrt left a comment

Choose a reason for hiding this comment

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

Please see my comments.

@@ -1,5 +1,5 @@
displayName: AI Actions
/execute/{actionConfiguration}:
displayName: AI actions

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Answered in DM - the short answer is that the old route still works)

/actions:
get:
displayName: List action configurations
description: Returns a list of action configurations
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
description: Returns a list of action configurations
description: Returns a list of action configurations.

description: Returns a list of action configurations
queryParameters:
limit:
description: The number of results per page
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we are inconsistent in putting fullstops at the end of query paramater descriptions, but the ones with fullstop largely prevail.
I'd add fullstops then, even though these are not full sentences. Alternatively, we'd want to go through all descriptions and amend them to have fullstops in sentences and no fullstops in nominal sentence fragments

type: string
text:
type: string[]
ActionTypeListWrapper:
Copy link
Contributor

@dabrt dabrt Feb 25, 2025

Choose a reason for hiding this comment

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

Again, we are inconsistent in providing descriptions for objects and for object properties. For close to 2 thousand elements that could have a description key (objects and properties), there is a little over a thousand descriptions.
What's the convention?

@mnocon
Copy link
Contributor Author

mnocon commented Feb 27, 2025

@julitafalcondusza @dabrt full stops and descriptions added in:

Copy link
Contributor

@dabrt dabrt left a comment

Choose a reason for hiding this comment

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

Please see my comments

</a>
</td>
<td></td>
<td>JSON object with only RefineText property.</td>
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
<td>JSON object with only RefineText property.</td>
<td>JSON object with only the RefineText property.</td>

</a>
</td>
<td></td>
<td>JSON object with only a GenerateAltText property.</td>
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
<td>JSON object with only a GenerateAltText property.</td>
<td>JSON object with only the GenerateAltText property.</td>

</a>
</td>
<td></td>
<td>JSON object with only AltText property.</td>
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
<td>JSON object with only AltText property.</td>
<td>JSON object with only the AltText property.</td>

</a>
</td>
<td></td>
<td>JSON object with only RefineText property.</td>
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
<td>JSON object with only RefineText property.</td>
<td>JSON object with only the RefineText property.</td>

</a>
</td>
<td></td>
<td>JSON object with only ActionTypeList property.</td>
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
<td>JSON object with only ActionTypeList property.</td>
<td>JSON object with only the ActionTypeList property.</td>

-
<span>
</span>
JSON object with only RefineText property.</span>
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
JSON object with only RefineText property.</span>
JSON object with only the RefineText property.</span>

-
<span>
</span>
JSON object with only RefineText property.</span>
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
JSON object with only RefineText property.</span>
JSON object with only the RefineText property.</span>

-
<span>
</span>
JSON object with only AltText property.</span>
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
JSON object with only AltText property.</span>
JSON object with only the AltText property.</span>

-
<span>
</span>
JSON object with only ActionTypeList property.</span>
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
JSON object with only ActionTypeList property.</span>
JSON object with only the ActionTypeList property.</span>

-
<span>
</span>
Object describing the criteria used to search for Action Conigurations.</span>
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
Object describing the criteria used to search for Action Conigurations.</span>
Object describing the criteria used to search for Action Configurations.</span>

@mnocon mnocon merged commit 91cec13 into master Mar 5, 2025
7 checks passed
@mnocon mnocon deleted the ai-rest branch March 5, 2025 09:51
mnocon added a commit that referenced this pull request Mar 5, 2025
* [REST] New AI endpoints

* Review fixes - part 1

* Regenerated responses

* Rebuild the reference

* Added fullstops.

* Added descriptions

* Regemerated the reference

* Description fixes

* Regenerated the reference
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.

4 participants