Skip to content
This repository was archived by the owner on Mar 22, 2025. It is now read-only.

Conversation

@lmas
Copy link
Owner

@lmas lmas commented Feb 3, 2025

Let's finish up the work until next week and review the code.

Purpose of the review:

  • Finding obvious errors or problems in the code to be merged in (we only want to merge in stable code that won't cause problems for any one else).
  • Fix all errors and get green checkmarks for all the automatic tests and linters.

Activity log:

  • about 1h creating this PR and doing first pass review
  • 1h messing around with the review and reverting rename commit

SimonPergel and others added 30 commits January 21, 2025 10:25
In order to get automatic tests when commiting code
Update for the automatic tests
Copy link
Collaborator

@Pake-TU Pake-TU left a comment

Choose a reason for hiding this comment

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

First review, I've been a little pedantic with the empty rows, but I've tried not to comment much about things Alexander brought up

EDIT: I think I've chosen the wrong setting for my review, it should've been a comment

@lmas
Copy link
Owner Author

lmas commented Feb 4, 2025

@lmas lmas mentioned this pull request Feb 10, 2025
9 tasks
Repository owner deleted a comment from Pake-TU Feb 11, 2025
t.set_SEKprice(w, r)
case "desired_temp":
t.set_desiredTemp(w, r)
case "MinTemperature":
Copy link
Owner Author

@lmas lmas Feb 12, 2025

Choose a reason for hiding this comment

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

An observation here, there's no need for action.
These aren't function names and didn't need to be updated. I admit I could have spent more time writing better comments and clarify this better.

Have you thought about any side effects this change may have? This block of code is part of your system's public API.
Changing these names here resulted in a change in the API and broke for example the GUI app and the influxdb system (as both have hard-coded dependencies on the previous names).

This is why we do code reviews, to find oversights like this.

@lmas lmas requested a review from Pake-TU February 14, 2025 11:37
@lmas lmas closed this Feb 14, 2025
@lmas
Copy link
Owner Author

lmas commented Feb 14, 2025

Closing, want to merge in to new dev branch instead and preserve the stability in master.

@lmas lmas mentioned this pull request Feb 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants