Addedd external temp and si

Thank you.

I am a novice and learnt python to make the addition to for my Davis station. This was my first programming and first use of GitHub and creating pulling request. The two commits are because of my lack of experience. This was my first pull request and I do not know and was not aware that there will be to commits. I know why it is there , but I though that my pull request will not include my last part of the correction. When I was writing the code I was writing with some specific features for my station. For example in my code, two temps are compared to detect if pool heater is On and then store time when pool heater was on and when was off. . This is why you see in the first commit names like pool and pool heater. After I made all the changes I thought that maybe other people will benefit from the extra temp probes and si conversion and decided to remove my specific features. I must have left some my last correction which was reported as another commit. I apologize, but I am still learning.

I appreciate your suggestions. I wish you answered me in August when I still had fresh memory in python programming. It will take me more time now than it would in August to separate the additions and rewrite the code. Unfortunately I do not have much time now. Maybe in December.

I have however one question

I assume that your suggestions to call functions that take the unit would make the programming more professional and neat but am I wrong that it will not change the functionality.

It works and was tested. I am afraid that I can break something that works. I am not experienced programmer and during writing my codes I made many mistakes that broke the functionality.

Regards

Andrzej

From: Bob Paauwe notifications@github.com Sent: October 31, 2020 3:45 PM To: bpaauwe/udi-davis-poly udi-davis-poly@noreply.github.com Cc: Andrzej Kajetanowicz github@anola.info; Author author@noreply.github.com Subject: Re: [bpaauwe/udi-davis-poly] Addedd external temp and si (#2)

@bpaauwe requested changes on this pull request.

Thanks for submitting the changes to add these features. I do have a number of suggestions that I'd like to see before accepting this.

I'd prefer not to get commits that break the code with additional commits that then fix it. You have the opportunity to test and fix before sending the merge request. So for some thing like this, please squash the two commits together before making the merge request.
I'd prefer it if features were separated into different commits. You're adding two features so make each feature addition a separate commit with a reasonably descriptive commit message.
Adding the ability to display in si units for day/month/year is a good idea, but the way you did that adds a lot of mostly duplicate code. I think it would be better to call functions that take the units, do the conversion if necessary. Something like this:

def temperature (self, units, value): if units == 'us': return value else: return round((value - 32) / 1.8, 1)

then in day/month/year ... if 'temp_day_high_f' in obs: self.update_driver('GV0', uom.temperature(units, obs['temp_day_high_f'])

This would make for a much smaller set of changes and it would make it easier to review the changes.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/bpaauwe/udi-davis-poly/pull/2#pullrequestreview-521160701 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AD5EFC4OUZRHIBJ2W5ZZ2WLSNRLKBANCNFSM4QNROBTQ . https://github.com/notifications/beacon/AD5EFC7B5MXNN3CAO3SL4RLSNRLKBA5CNFSM4QNROBT2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOD4IEP7I.gif

bpaauwe / udi-davis-poly

Addedd external temp and si #2