Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
s2t2
left a comment
There was a problem hiding this comment.
Hi @youneedgreg thanks for the PR! this looks generally good.
I just have two minor questions about formatting.
| 'supply_water_setpoint', | ||
| 'supply_water_temperature_sensor', | ||
| 'heating_request_count', | ||
| ], |
There was a problem hiding this comment.
I'm curious, were these indentation changes made by the auto-formatter during pre-commit hooks? Or are these indentation changes made manually?
Can you please confirm if you have installed pre-commit hooks?
https://google.github.io/sbsim/contributing/#pre-commit-hooks
If our auto-formatter is responsible, we will want to update it (in a separate issue / PR) to use a hanging indentation of four spaces (to match Google internal style rules).
There was a problem hiding this comment.
The indentation changes were made manually as pre-commit hooks were not installed locally. I've confirmed the code uses 4-space hanging indentation, which aligns with the Google style guide.
There was a problem hiding this comment.
OK, I saw those manual indentation changes were still in place, so I just restored the hanging indentation to use four spaces.
s2t2
left a comment
There was a problem hiding this comment.
@youneedgreg thanks for your reply, and for making those updates. I saw the device_type method is still lacking a property decorator, so if you could please implement that, then this PR will be ready for merge!
Fixes #135
All relevant test files
Note: No changes were made for device_filepath as it was not found in the codebase.