-
Notifications
You must be signed in to change notification settings - Fork 45.3k
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
docs(platform): Example block #9543
base: dev
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for auto-gpt-docs-dev canceled.
|
Here's the code health analysis summary for commits Analysis Summary
|
✅ Deploy Preview for auto-gpt-docs canceled.
|
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 is missing
- _api.py and _auth.py file formats for when you have more than one related block
- How to split blocks up into different files that are related
- best practices/examples of inputs and output formatting such as yielding the list and the items in the list as two separate things
im sure I’ll think of more but this is a good start
I've made all the changes you've suggested. Though still need to make examples of these:
|
The PR has several issues that need addressing before it can be approved:
|
The PR is well structured and follows most requirements but has some notable issues:
However, the implementation itself appears solid with appropriate documentation and the author explicitly states this is meant as an example PR for best practices, acknowledging it's not intended for merging. The changes are well documented and the implementation appears complete. |
self.status_code = status_code | ||
|
||
|
||
class ExampleClient: |
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.
Why is this client boilerplate needed?
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.
The purpose of this PR is capture the best practices of adding a new integration block. Creating a client seems to be how we have implemented this with other bocks.
Do you feel this is not best practice?
If so what would you suggest?
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.
Yes I think it makes sense when the SDK support is missing.
But stuff here getting copied over and over without any refactoring: these ended up being duplicated in the HTTP client part:
- Setting up Request class
- Set content type, passing header to Authorization
- Adding to trusted origin (which is not actually needed)
Other issues:
-
Return the value of Dict which should be properly typed. (also the seems like our convention is using dict/list | None as opposed to Dict List Optional)
-
Creating a custom exception, then catching it within the same class, shouldn't this be the other way, catching an HTTP error and wrapping it to the custom exception (if it's even needed)?
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.
we can use pydantic objects and that should be preferred except when there's a fuck off big response from the API. That case should be model dumpted to a "dict" and attached as a raw object. Github Apollo/smartlead do this
payload: dict = SchemaField(hidden=True) | ||
|
||
class Output(BlockSchema): | ||
event_data: dict = SchemaField( |
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.
Does this have to be a single event_data and be parsed manually later by another block?
The best practice is to avoid returning complex object as much as possible.
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.
@Pwuts can you help with best practice here please
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.
see generic webhook block pr
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.
Sorry I've been adding comment and forgot to submit it
description="The context of the greeting", | ||
placeholder="Enter a context", | ||
default="The user is looking for an inspirational greeting", | ||
# Hidden fields are not shown in the UI by default |
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.
so what's the difference between advanced
and hidden
?
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.
Advanced are shown when user clicks Advanced
toggle, I guess hidden are permanently hidden.
But then, does it need to be SchemaField
?
That means it's still sent to the frontend unless this is explicitly avoided and possibly can be force-provided with data from the user, is this ok?
class Input(BlockSchema): | ||
# The payload field is hidden because it's automatically populated by the webhook | ||
# system rather than being manually entered by the user | ||
payload: dict = SchemaField(hidden=True) |
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 should be more specifically typed if possible
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 second this
autogpt_platform/backend/backend/integrations/webhooks/example.py
Outdated
Show resolved
Hide resolved
events: list[str], | ||
ingress_url: str, | ||
secret: str, | ||
) -> tuple[str, dict]: |
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.
- We return weakly typed
dict
here; if function returns a specific structure it'd be better to returnTypedDict
/class
/namedtuple
or even pydantic model. - Why return empty string or id as separate item? Can't we just return it inside the
dict
?
autogpt_platform/frontend/src/components/integrations/credentials-input.tsx
Outdated
Show resolved
Hide resolved
description="The context of the greeting", | ||
placeholder="Enter a context", | ||
default="The user is looking for an inspirational greeting", | ||
# Hidden fields are not shown in the UI by default |
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.
Advanced are shown when user clicks Advanced
toggle, I guess hidden are permanently hidden.
But then, does it need to be SchemaField
?
That means it's still sent to the frontend unless this is explicitly avoided and possibly can be force-provided with data from the user, is this ok?
|
||
|
||
# Mock credentials for testing Example API integration | ||
TEST_CREDENTIALS = APIKeyCredentials( |
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'd explicitly mention not to include real keys here to not leak them.
if custom_requests: | ||
self._requests = custom_requests | ||
else: | ||
headers: Dict[str, str] = { |
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.
headers: Dict[str, str] = { | |
headers: dict[str, str] = { |
class Input(BlockSchema): | ||
# The payload field is hidden because it's automatically populated by the webhook | ||
# system rather than being manually entered by the user | ||
payload: dict = SchemaField(hidden=True) |
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 second this
Co-authored-by: Reinier van der Leer <[email protected]>
42feeab
to
5af718c
Compare
The PR is well structured and follows most guidelines, but has some issues: 1) While the PR title has a scope in parentheses, it doesn't follow the exact conventional commit format which should be 'feat(scope): message' rather than docs(scope). 2) There is a partial checklist where some sections are filled but others are left incomplete (like the test plan section which has checkboxes but no indication if they were completed). 3) Changes in data/*.py files correctly handle user_id access. |
The PR is well structured and follows most requirements but has some minor issues that need addressing: 1) While the PR title has a scope 'docs(platform)', it appears this is more of a feat/feature PR since it's adding new example blocks and functionality rather than just documentation 2) The checklist in the template is partially filled - only 2 items are checked while others remain unchecked without explicit removal of inapplicable sections. |
The PR is well structured and follows the established patterns, but has a few issues that need to be addressed:
However, these are relatively minor issues and the actual code changes look solid - proper user_id handling in data files, good documentation, and follows established patterns. The author also explicitly asks for feedback to improve it further. |
This PR introduces
ExampleBlock
andExampleTriggerBlock
to showcase best practices for creating blocks, managing credentials, and integrating webhooks. Key changes:Purpose: This is an example PR, not intended for merging. We’re requesting reviews to ensure it captures the best practices for block creation and serves as an optimal reference for contributors.
This includes nits on docstrings and comments
Checklist