added Energiegemeinschaft#343
Conversation
added sensors and statistics for roles G001 and G003
|
@AndiVienna91 having that many changes in your PR doesn't look right. |
|
Why did you basically remove all comments and docstrings? 🤔 |
|
my question exactly. This doesn't seem like a minimal change for the issue addressed. Your intention is very much welcome, since it's a feature that has been requested oftentimes, but please try to make this most minimal so we can actually check it. Removing comments without reason is not a good way of contributing. |
DarwinsBuddy
left a comment
There was a problem hiding this comment.
take care of formatting and don't remove comments
make this a minimal change please
|
I had the code cleaned up by an AI after some experimentation, the comments apparently got removed in the process. They have now been restored. |
DarwinsBuddy
left a comment
There was a problem hiding this comment.
I really value your input, but this is still not minimal.
I do get that using LLMs for this kind of work is appealing and useful, but if everyone changes comments arbitrarily there is no such sense in having comments.
A PR claming of adding something should not change something that is unrelated.
If that's not possible to prompt to an AI successfully, I suggest going over the changes one by one in your favourite Difftool and prepare this PR properly by squashing.
|
No offense, but I’m not going to keep reworking a piece of functioning code just to match your preferences. After trying several different approaches, this version turned out to be the most reliable and stable one from my perspective, and it meets my requirements. You’re welcome to extract and integrate only what you consider essential into your own code. If it works, the community benefits—if not, then so be it. That said, I do wonder why these features haven’t already been implemented if there’s clearly demand for them. In any case, I’m stepping out at this point. Wishing you all the best moving forward. |
|
I can understand your frustration, but on the other side is also frustrating for the reviewers.
well, removing comments or completely rewriting them is not good. I mean okay, you added now docstrings for previously undocumented functions - credits for that - but you removed, for example, pylint statements, which have a specific meaning. These are not just preferences but information for the test-system.
yes, it meets your requirements. But remember, there are a lot more people using this tool and when programming on such projects, you have to keep their requirements in mind too.
Several reasons, for example:
|
added sensors and statistics for roles G001 and G003