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.

Warning:

  • This PR accidentality affected the comfortstat source, so this issue gets double time to solve any extra merge issues.

Activity log:

  • 0.5h making an initial quick review
  • 1h trying to undo the removal commit of the comfortstat dir but messed up the whole branch massively, revert revert revet.

@lmas
Copy link
Owner Author

lmas commented Feb 3, 2025

@SimonPergel maybe you could continue doing this review, inspecting the code for problems the same way I did it?

@lmas lmas self-assigned this Feb 3, 2025
@lmas lmas requested a review from Pake-TU February 3, 2025 10:14
Copy link
Owner Author

@lmas lmas left a comment

Choose a reason for hiding this comment

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

Initial commits.

@lmas lmas removed the request for review from Pake-TU February 3, 2025 10:27
@lmas
Copy link
Owner Author

lmas commented Feb 4, 2025

I managed to undo the commit that removed the comfortstat dir, in commit 85cccb0.
A side effect is that now it looks like I co-authored all commits from that one, down to commit 29d8424, and that the tests are failing all the time on the comfortstat tests.
But we can merge this PR now without risk of accidently removing comfortstat.

This created a huge mess with the branch and the history on this PR.
Lesson learned: Don't fuck with pushed commit history.

@lmas lmas mentioned this pull request Feb 10, 2025
9 tasks
@lmas
Copy link
Owner Author

lmas commented Feb 11, 2025

@SimonPergel @gabaxh still waiting on review comments on pake's code.

@gabaxh
Copy link
Collaborator

gabaxh commented Feb 11, 2025

This review will be quite picky as I did not find anything big that I do not agree with but still want to leave some reviews.

@lmas
Copy link
Owner Author

lmas commented Feb 14, 2025

Closing, merging into dev instead.

@lmas lmas closed this 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