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

Solution #497

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

Solution #497

wants to merge 2 commits into from

Conversation

mshmygel
Copy link

@mshmygel mshmygel commented Jan 9, 2025

image

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor 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 for your hard work on this project! While your code is well-structured and meets many of the task requirements, there are a few critical issues that need to be addressed before we can approve the pull request:

  1. Awaiting Coroutines: The run_sequence and run_parallel functions return coroutines, which need to be awaited to execute them immediately. Please ensure that these are correctly awaited in your code.

  2. run_program Method: This method is currently being passed a coroutine from run_parallel, which is not handled correctly. You need to adjust the method to handle coroutines or change how programs are defined and executed.

  3. Message Handling: The run_program method attempts to await a list, which is not valid. You should iterate over the list and await each Message individually if they are asynchronous operations.

  4. Imports and Definitions: Ensure that Message and MessageType are correctly defined and imported in the message.py file to avoid errors when using them.

Once these issues are resolved, your code will be in great shape! Keep up the excellent work, and don't hesitate to reach out if you have any questions. 😊

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

@@ -1,64 +1,79 @@
import time
import asyncio

from .message import MessageType

Choose a reason for hiding this comment

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

Ensure that MessageType is correctly defined in the message.py file. If it's not defined or imported correctly, it will cause an error when trying to use message_type.name in the send_message methods.

print("=====RUNNING PROGRAM======")
for msg in program:
self.send_msg(msg)
await program

Choose a reason for hiding this comment

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

The run_program method currently attempts to await a list, which is not valid. You should iterate over the program list and await each Message individually if they are asynchronous operations.

@@ -2,48 +2,53 @@
import string
from typing import Protocol

from .message import Message, MessageType
from app.iot.message import Message, MessageType

Choose a reason for hiding this comment

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

Ensure that Message and MessageType are correctly defined in the message.py file. If they are not defined or imported correctly, it will cause errors when using them in the code.


# run the programs
service.run_program(wake_up_program)
service.run_program(sleep_program)
await service.run_program(run_parallel(wake_up_program, sleep_program))

Choose a reason for hiding this comment

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

The run_program method in IOTService is designed to take a list of Message objects, but here it's being passed the result of run_parallel, which is a coroutine. You need to adjust the run_program method to handle coroutines or change the way programs are defined and executed.

MessageType.PLAY_SONG,
"Rick Astley - Never Gonna Give You Up"
),
)

Choose a reason for hiding this comment

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

The run_sequence function returns a coroutine, which should be awaited. Ensure that when you call run_sequence, you await its result if you intend to execute it immediately.

),
service.send_msg(toilet_id, MessageType.FLUSH),
service.send_msg(toilet_id, MessageType.CLEAN),
)

Choose a reason for hiding this comment

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

Similarly, ensure that the sleep_program coroutine is awaited if you intend to execute it immediately.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor 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 for your submission! Your code is on the right track, but there are a few critical issues that need attention to ensure full functionality:

  1. Awaiting Coroutines: The run_sequence and run_parallel functions return coroutines that need to be awaited to execute them immediately. Make sure to await wake_up_program and sleep_program to ensure they run as expected.

  2. run_program Method: This method is currently attempting to await a list directly, which is not valid. You should iterate over the program list and await each coroutine individually. Consider using await asyncio.gather(*program) if program is a list of coroutines.

  3. Handling Coroutines: The run_program method is passed a coroutine from run_parallel, which is not handled correctly. Adjust the method to handle coroutines properly or redefine how programs are executed.

Your efforts are appreciated, and these adjustments will help you improve your understanding and implementation of asynchronous programming. Keep up the good work, and make sure to address these issues in future iterations to enhance your learning experience. 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

Comment on lines +46 to +48
async def run_program(self, program: list[Message]) -> None:
print("=====RUNNING PROGRAM======")
for msg in program:
self.send_msg(msg)
await program

Choose a reason for hiding this comment

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

The run_program method attempts to await a list, which is not valid. You should iterate over the program list and await each Message individually if they are asynchronous operations.

Comment on lines +31 to +41
wake_up_program = run_sequence(
run_parallel(
service.send_msg(hue_light_id, MessageType.SWITCH_ON),
service.send_msg(speaker_id, MessageType.SWITCH_ON),
),
service.send_msg(
speaker_id,
MessageType.PLAY_SONG,
"Rick Astley - Never Gonna Give You Up"
),
)

Choose a reason for hiding this comment

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

The run_sequence function returns a coroutine and should be awaited to execute it immediately. Ensure that wake_up_program is awaited.

Comment on lines +43 to +50
sleep_program = run_sequence(
run_parallel(
service.send_msg(hue_light_id, MessageType.SWITCH_OFF),
service.send_msg(speaker_id, MessageType.SWITCH_OFF),
),
service.send_msg(toilet_id, MessageType.FLUSH),
service.send_msg(toilet_id, MessageType.CLEAN),
)

Choose a reason for hiding this comment

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

The run_sequence function returns a coroutine and should be awaited to execute it immediately. Ensure that sleep_program is awaited.


# run the programs
service.run_program(wake_up_program)
service.run_program(sleep_program)
await service.run_program(run_parallel(wake_up_program, sleep_program))

Choose a reason for hiding this comment

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

The run_program method is passed a coroutine from run_parallel, which is not handled correctly. You need to adjust the method to handle coroutines or change how programs are defined and executed.

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