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

[SYNPY-1544] potential changes to mixin #1153

Merged

Conversation

BryanFauble
Copy link
Contributor

@BryanFauble BryanFauble commented Jan 14, 2025

@BWMac and I are working together to discuss ways in which we will implement this feature

@BryanFauble BryanFauble requested a review from a team as a code owner January 14, 2025 18:56
@pep8speaks
Copy link

pep8speaks commented Jan 14, 2025

Hello @BryanFauble! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 23:89: E501 line too long (109 > 88 characters)
Line 34:89: E501 line too long (108 > 88 characters)
Line 50:89: E501 line too long (108 > 88 characters)
Line 67:89: E501 line too long (108 > 88 characters)
Line 240:89: E501 line too long (140 > 88 characters)
Line 277:89: E501 line too long (140 > 88 characters)
Line 309:89: E501 line too long (96 > 88 characters)

Comment last updated at 2025-01-14 20:51:40 UTC

@BWMac BWMac requested review from BWMac and removed request for a team January 14, 2025 20:10
Copy link
Contributor

@BWMac BWMac left a comment

Choose a reason for hiding this comment

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

Thank you, this is much better than what I had previously. I like using AgentPrompt as the async communicator, makes much more sense.

@@ -151,129 +195,156 @@ def fill_from_dict(self, async_job_status: dict) -> "AsynchronousJobStatus":
return self


class AsynchronousJob:
async def send_job_and_wait_async(
Copy link
Contributor

Choose a reason for hiding this comment

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

should these be in a separate script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. This would probably make sense to live within a py script at synapseclient.api

Copy link
Contributor

@BWMac BWMac Jan 14, 2025

Choose a reason for hiding this comment

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

I can do this before merging

Copy link
Contributor

@BWMac BWMac Jan 14, 2025

Choose a reason for hiding this comment

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

@BryanFauble Actually, maybe that isn't possible. If we move get_job_async we need to move the AsynchronousJobStatus and AsynchronousJobState classes to avoid a circular import issue, but that doesn't make a whole lot of sense to me to have those classes in a _services.py script. I think I will leave these where they are for now.

Copy link
Contributor

@BWMac BWMac left a comment

Choose a reason for hiding this comment

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

Thank you, this is much better than what I had previously. I like using AgentPrompt as the async communicator, makes much more sense.

@BWMac BWMac merged commit 165570c into synpy-1544-synapse-agent-oop-model Jan 14, 2025
10 of 16 checks passed
@BWMac BWMac deleted the synpy-1544-potential-changes-to-mixin branch January 14, 2025 20:59
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.

3 participants