Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Feature/issue 39 #68

Merged
merged 8 commits into from
Jan 20, 2024
Merged

Feature/issue 39 #68

merged 8 commits into from
Jan 20, 2024

Conversation

vggonzal
Copy link
Contributor

@vggonzal vggonzal commented Dec 18, 2023

Github Issue: #39

Description

Clean up code

Overview of work done

Removed commented out code
Removed unused parameters
Cleaned up todo comments

Overview of verification done

Same unit tests

Overview of integration done

N/A

PR checklist:

  • Linted
  • Updated unit tests
  • Updated changelog
  • Integration testing

See Pull Request Review Checklist for pointers on reviewing this pull request

Copy link
Member

@frankinspace frankinspace left a comment

Choose a reason for hiding this comment

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

Inline comments are ok, those didn't need to be removed. Code that is commented out is what is unnecessary

@vggonzal
Copy link
Contributor Author

Inline comments are ok, those didn't need to be removed. Code that is commented out is what is unnecessary

Do you want me to put them all back? most of them are a bit unnecessary, the code is kind of self explanatory. But I can put them back if you think they help with code maintenance.

Copy link
Collaborator

@torimcd torimcd left a comment

Choose a reason for hiding this comment

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

Agree with Frank's comments, otherwise looks good to me

@frankinspace frankinspace merged commit 3d78754 into develop Jan 20, 2024
@frankinspace frankinspace deleted the feature/issue-39 branch January 20, 2024 00:42
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants