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

Refactor Light Control Code #39

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

melissadjadoun
Copy link

This pull request aims to clean up the light.py control module by removing unnecessary methods and MQTT parameters. The changes include:

  • Removed redundant methods: Identified and deleted methods that were not contributing to the functionality of the LED control.

  • Streamlined MQTT parameters: Eliminated unused MQTT parameters to simplify the communication process and improve code maintainability.

  • Improved code readability: Refactored parts of the code to enhance readability and reduce complexity.

@ethanjli
Copy link
Member

ethanjli commented Jul 4, 2024

Is this PR intended to replace #35? If so, would it be acceptable if you merge your branch onto Oumayma's branch for #35 and close this PR? That would allow me to see what changes you've made over Oumayma's work.

@melissadjadoun
Copy link
Author

@ethanjli The code for the light in PR #35 was not functioning correctly, and I was unable to identify the exact issue. Therefore, I created a new branch from the initial branch and began refactoring incrementally. I tested the changes on the Planktoscope in real-time after each modification or removal to observe their impact on the light.
As requested, I have merged my branch into Oumayma's branch, resolving any conflicts that arose during the merge process. This allows you to review the changes I’ve made.

@ethanjli
Copy link
Member

ethanjli commented Jul 8, 2024

Thanks for the clarification about why you made a new branch for testing - that makes sense to me. I don't see your changes in #35 yet - instead, it looks like you've merged Oumayma's branch into your branch and pushed the result (which shows up as changes in this PR). Can you please try to merge your branch into Oumayma's branch, perform testing to ensure that the merged branch still works as you expect, and push the result (which should show up as changes in #35)?

@ethanjli
Copy link
Member

Based on @melissadjadoun 's message to me in Slack about problems which @Oumayma-hy encountered in attempting to merge this PR's branch into PR #35's branch, I think we will give up on trying to do that; instead, we will close PR #35 and do all remaining work for the light module refactoring in this branch. And once that work is finished, we will merge this PR (39).

@melissadjadoun
Copy link
Author

@ethanjli I have completed the task of comparing the code changed in PR 39 (the result of merging Oumayma's branch into my branch) with the changes originally made on my branch. I have fixed any issues where lines of code were incorrectly added or removed during the merge.
After making the necessary adjustments, I tested the code on the PlanktoScope, and I can confirm that the LED control functionality continues to work with the new code in PR 39.

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