This is a list of all comments for DIALIN-DEN-4308-1. Review Summary: No summary ---------------------------------------- File: dialin/hd/valves.py Revision Comment by plucia on 30 September 2020, 09:42 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c4950 These should be in common Reply by Dara Navaei on 30 September 2020, 11:20 > Where is common? Reply by plucia on 30 September 2020, 11:41 > It's here: > [http://dvm-linux02:7990/projects/VV/repos/dialin/browse/dialin/common] Reply by Dara Navaei on 01 October 2020, 09:29 > Done Reply by plucia on 02 October 2020, 10:20 > RESOLVED. Revision Comment by plucia on 30 September 2020, 09:44 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c4952 To be consistent with other classes, I'd recommend you subclass enum.Enum for these so the names and values can be accessed more easily. See temperature_sensors.py for an example Reply by Dara Navaei on 01 October 2020, 17:26 > Done Reply by plucia on 02 October 2020, 10:19 > RESOLVED Revision Comment by Sean Nash on 29 September 2020, 15:57 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c4942 Shouldn't there be a set of the properties for each valve? Reply by Dara Navaei on 01 October 2020, 17:26 > Done Reply by Sean Nash on 05 October 2020, 11:10 > RESOLVED. Revision Comment by Sean Nash on 05 October 2020, 11:07 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c5159 Can't we just range check vlv_ID instead of using try? Reply by Dara Navaei on 05 October 2020, 15:02 > This is not just to check the range of the valves. All the > other enums that we have such as valves positions and valves > states also need to be tried to make sure nothing out of > range was selected. Reply by Dara Navaei on 08 October 2020, 09:57 > I removed try and except and added has_value method. Reply by Sean Nash on 08 October 2020, 10:14 > RESOLVED. Revision Comment by plucia on 30 September 2020, 09:42 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c4951 Please make the handler private (e.g. _handler instead of handler) Reply by Dara Navaei on 30 September 2020, 11:17 > Done Reply by plucia on 01 October 2020, 17:30 > RESOLVED Revision Comment by Sean Nash on 29 September 2020, 15:58 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c4943 Should be an array of records (like in f/w) and this received record should be saved to the appropriate record in the array. Reply by Dara Navaei on 01 October 2020, 17:26 > Done Reply by Sean Nash on 05 October 2020, 11:08 > RESOLVED. Revision Comment by plucia on 30 September 2020, 10:57 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c4982 If you intend to keep this handler, please keep it _private and publish the data it receives Reply by Dara Navaei on 30 September 2020, 15:30 > This handler will be removed. Reply by Dara Navaei on 02 October 2020, 16:31 > It has been removed. Reply by plucia on 08 October 2020, 09:18 > RESOLVED ---------------------------------------- File: dialin/squish/denaliMessages.py Revision Comment by pmontazemi on 05 October 2020, 15:43 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c5209 Last ")" comes after vPWM)). Reply by pmontazemi on 06 October 2020, 09:40 > RESOLVED. Revision Comment by pmontazemi on 28 September 2020, 09:44 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c4925 Remove extra line. Reply by Dara Navaei on 30 September 2020, 10:20 > Done Reply by pmontazemi on 30 September 2020, 14:54 > RESOLVED. Revision Comment by pmontazemi on 28 September 2020, 09:44 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c4926 Align with left "(" Reply by Dara Navaei on 30 September 2020, 11:14 > Done Reply by pmontazemi on 30 September 2020, 14:51 > RESOLVED. Revision Comment by pmontazemi on 28 September 2020, 09:44 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c4927 Remove extra line. Reply by Dara Navaei on 30 September 2020, 10:21 > Done Reply by pmontazemi on 30 September 2020, 14:51 > RESOLVED. Revision Comment by pmontazemi on 28 September 2020, 09:45 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c4928 Remove extra line. Reply by Dara Navaei on 30 September 2020, 10:22 > Done Reply by pmontazemi on 30 September 2020, 14:53 > RESOLVED. Revision Comment by pmontazemi on 28 September 2020, 09:46 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c4929 Remove extra line. Reply by Dara Navaei on 30 September 2020, 10:24 > Done Reply by pmontazemi on 30 September 2020, 14:52 > RESOLVED. Revision Comment by pmontazemi on 28 September 2020, 09:46 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c4930 Remove extra line. Reply by Dara Navaei on 30 September 2020, 10:23 > Done Reply by pmontazemi on 30 September 2020, 14:52 > RESOLVED. Revision Comment by pmontazemi on 28 September 2020, 09:46 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c4931 Remove extra line. Reply by Dara Navaei on 30 September 2020, 10:23 > Done Reply by pmontazemi on 30 September 2020, 14:52 > RESOLVED. Revision Comment by pmontazemi on 28 September 2020, 09:47 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c4932 Align with left "(". Reply by Dara Navaei on 30 September 2020, 11:15 > Done Reply by pmontazemi on 30 September 2020, 14:53 > RESOLVED. Revision Comment by pmontazemi on 28 September 2020, 09:48 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c4933 Align with left "(". Reply by Dara Navaei on 30 September 2020, 10:27 > Done Reply by pmontazemi on 30 September 2020, 14:52 > RESOLVED. Revision Comment by pmontazemi on 28 September 2020, 09:49 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c4934 Remove extra spaces at the end of argument list between (). Reply by Dara Navaei on 30 September 2020, 10:28 > Done Reply by pmontazemi on 30 September 2020, 14:52 > RESOLVED. Revision Comment by pmontazemi on 28 September 2020, 09:49 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c4935 Align with left "(". Reply by Dara Navaei on 30 September 2020, 14:59 > Done Reply by pmontazemi on 30 September 2020, 15:10 > RESOLVED. Revision Comment by pmontazemi on 28 September 2020, 09:58 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c4936 Align with left "(". Reply by Dara Navaei on 30 September 2020, 14:59 > Done Reply by pmontazemi on 30 September 2020, 15:10 > RESOLVED. ---------------------------------------- File: dialin/squish/messageBuilder.py Revision Comment by pmontazemi on 28 September 2020, 09:58 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c4937 Remove extra line. Reply by Dara Navaei on 30 September 2020, 10:28 > Done Reply by pmontazemi on 30 September 2020, 14:50 > RESOLVED. ---------------------------------------- File: tests/hd_valves_test.py Revision Comment by plucia on 30 September 2020, 15:14 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c5025 Any reason to keep this? Reply by Dara Navaei on 30 September 2020, 15:31 > During testing, I have been commenting and uncommenting code > to test different parts of code. Since this is in tests, does > that really matter? Reply by plucia on 02 October 2020, 10:23 > RESOLVED Revision Comment by plucia on 30 September 2020, 15:14 https://devapps.diality.us/cru/DIALIN-DEN-4308-1#c5026 Any reason to keep this? Reply by Dara Navaei on 30 September 2020, 15:31 > During testing, I have been commenting and uncommenting code > to test different parts of code. Since this is in tests, does > that really matter? Reply by plucia on 02 October 2020, 10:22 > RESOLVED --- ID: DIALIN-DEN-4308-1 https://devapps.diality.us/cru/DIALIN-DEN-4308-1 Title: DIALIN-DEN-4308_HD Valve Control (1 of 2) Statement of Objectives: State: Closed Summary: Author: Dara Navaei Moderator: Dara Navaei Reviewers: (1 active, 2 completed*) Sean Nash (*) plucia (*) pmontazemi