This is a list of all comments for CS-BUILD-0-2-4-1. Review Summary: No summary General Comment by Behrouz NematiPour on 12 December 2022, 12:09 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15193 Update all the function docstrings? Reply by Behrouz NematiPour on 13 January 2023, 16:50 > RESOLVED. > It has been addressed in other comments. General Comment by Behrouz NematiPour on 12 December 2022, 12:10 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15195 Update the code base in regards to the Diality Python coding standard. To be included : X:\Engineering\Denali\06- Software Design\Software Documentation\Coding Standards SOP-0058, Software Coding Standards, Python, Rev. 01.docx Reply by Behrouz NematiPour on 13 January 2023, 16:51 > RESOLVED > It has been addressed mostly and has some extra comments > about that. ---------------------------------------- File: cloud_sync.py Revision Comment by Behrouz NematiPour on 12 December 2022, 11:53 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15191 magic number? Reply by Behrouz NematiPour on 13 January 2023, 16:48 > Radu: > "Fixed" Reply by Behrouz NematiPour on 13 January 2023, 16:48 > RESOLVED Revision Comment by Behrouz NematiPour on 12 December 2022, 11:53 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15192 magic number? Reply by Behrouz NematiPour on 13 January 2023, 16:51 > Radu: > "Fixed" Reply by Behrouz NematiPour on 13 January 2023, 16:51 > RESOLVED ---------------------------------------- File: cloudsync/busses/file_input_bus.py Revision Comment by Sean Nash on 14 December 2022, 10:40 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15201 All .py files should have a file header. Reply by Behrouz NematiPour on 13 January 2023, 16:03 > Radu: > "Added the comment" Reply by Sean Nash on 16 January 2023, 14:26 > RESOLVED. Revision Comment by Sean Nash on 14 December 2022, 10:41 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15202 Classes should have a header w/ brief description. Reply by Behrouz NematiPour on 13 January 2023, 16:04 > Radu: > "Added the comment" Reply by Sean Nash on 16 January 2023, 14:28 > This class now has a header/brief, but other classes do not > appear to have been updated. Reply by Behrouz NematiPour on 16 January 2023, 15:54 > Radu: > "The Team is in the progress of updating and refactoring. > and classes and functions may need to change to better be > able to test. Therefore all the missing documentation > will be addressed in upcoming build" Reply by Sean Nash on 17 January 2023, 15:41 > RESOLVED. Revision Comment by Sean Nash on 14 December 2022, 10:42 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15203 Functions should have a header w/ brief description and @param describing parameters and @return describing return value if applicable. Reply by Behrouz NematiPour on 13 January 2023, 16:01 > Radu: > "We are currently modifying functions and methods, to make > them easier for UnitTesting and it is changing. If you don't > mind we will address all the old and new functions when we > are done with our modifications and code cleanup to add the > the function documents" Reply by Sean Nash on 17 January 2023, 15:41 > RESOLVED. ---------------------------------------- File: cloudsync/busses/file_output_bus.py Revision Comment by Sean Nash on 14 December 2022, 10:44 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15204 Should be @return for our code documenting tool. Or is KBM using a different tool that uses this comment format? Reply by Behrouz NematiPour on 13 January 2023, 16:06 > Radu: > "Regarding the Diality Coding Standard, we have to comply > with the PIP257, which doesn't enforce to use "@" or ":", > regardless if it is Diality's preference, let us do it in the > next build." Reply by Sean Nash on 17 January 2023, 15:40 > RESOLVED. Revision Comment by Sean Nash on 14 December 2022, 10:45 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15205 @param and @return. Reply by Behrouz NematiPour on 13 January 2023, 16:07 > Radu: > "Regarding the Diality Coding Standard, we have to comply > with the PIP257, which doesn't enforce to use "@" or ":", > regardless if it is Diality's preference, let us do it in the > next build." Reply by Sean Nash on 17 January 2023, 15:40 > RESOLVED. ---------------------------------------- File: cloudsync/config/config.json Revision Comment by Behrouz NematiPour on 12 December 2022, 11:38 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15187 Why do we need the device's IP address? where is it being used and what happens if it changes? Reply by Sean Nash on 14 December 2022, 10:48 > Are these settings just an example of a configuration? > Should this file be part of code repo? Reply by Behrouz NematiPour on 13 January 2023, 16:08 > Radu: > - "It will be used for the device registration and > validation on the CloudSystem" > Behrouz: > - "This file is part of the code repo, and those were not > part of an example, those were actual hardcoded information > which later had to be set by MFG on the device and a sorry > has been created for that." Reply by Behrouz NematiPour on 13 January 2023, 16:14 > RESOLVED. Revision Comment by Behrouz NematiPour on 15 December 2022, 22:13 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15240 Please remove/empty any parameter value which depends on the device, server or something like that. and keep this file as an empty template. Same for serials, and names, ... Reply by Behrouz NematiPour on 13 January 2023, 16:15 > Radu: > "The parameters are now cleaned up and only the constant > parameters are kept." > "UI needs to fill in some parameters later and CS will fill > in/modify some others, regarding the QA or INT server being > used and the device information" Reply by Behrouz NematiPour on 13 January 2023, 16:16 > RESOLVED Revision Comment by Behrouz NematiPour on 15 December 2022, 22:12 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15239 the popular port of telnet, HTTP, and so on will be closed on the device. we had a discussion to change the CloudSync communication port to something like 5000. Please update. Reply by Behrouz NematiPour on 13 January 2023, 16:20 > Radu: > "The port is now changed to 5001" Reply by Behrouz NematiPour on 13 January 2023, 16:20 > RESOLVED ---------------------------------------- File: cloudsync/handlers/cs_mft_dcs_request_handler.py Revision Comment by Sean Nash on 14 December 2022, 10:57 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15210 Magic numbers. Reply by Behrouz NematiPour on 13 January 2023, 16:21 > Radu: > "fixed." Reply by Sean Nash on 16 January 2023, 14:34 > RESOLVED. Revision Comment by Sean Nash on 14 December 2022, 10:57 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15209 Magic numbers. Reply by Behrouz NematiPour on 13 January 2023, 16:21 > Radu: > - "fixed." > - "The 30000 and 31000 are a workaround for a bug which we > are currently working on and will be removed when fixed." Reply by Sean Nash on 16 January 2023, 14:34 > RESOLVED. Revision Comment by Sean Nash on 14 December 2022, 10:57 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15208 Magic number. Reply by Behrouz NematiPour on 13 January 2023, 16:43 > Radu: > "fixed." Reply by Sean Nash on 16 January 2023, 14:35 > RESOLVED. ---------------------------------------- File: cloudsync/handlers/outgoing/handler_cs_to_dcs.py Revision Comment by Sean Nash on 14 December 2022, 10:56 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15207 Magic number. Reply by Behrouz NematiPour on 13 January 2023, 16:20 > Radu: > "fixed." Reply by Sean Nash on 16 January 2023, 14:34 > RESOLVED. ---------------------------------------- File: cloudsync/handlers/uics_message.py Revision Comment by Behrouz NematiPour on 12 December 2022, 11:45 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15188 Please be consistent and rename the file to ui_cs_message.py like has been done with ui_cs_request_handler.py. Reply by Behrouz NematiPour on 13 January 2023, 16:25 > Radu: > "The ui_cs means the direction of the communication, and when > the direction doesn't matter w only put the entity names as > uics. but if it clarifies better will remove the uics to io " Reply by Behrouz NematiPour on 13 January 2023, 16:29 > RESOLVED ---------------------------------------- File: cloudsync/utils/helpers.py Revision Comment by Sean Nash on 14 December 2022, 11:00 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15212 Times are being multiplied by 1000 in a lot of places (magic number?). Are we converting seconds to milliseconds? I think replacing 1000 with a well named constant would help clarify. Reply by Behrouz NematiPour on 13 January 2023, 16:40 > Radu: > "Fixed" Reply by Sean Nash on 16 January 2023, 14:35 > RESOLVED. Revision Comment by Sean Nash on 14 December 2022, 10:59 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15211 Magic numbers. Reply by Behrouz NematiPour on 13 January 2023, 16:41 > Radu: > "fixed" Reply by Sean Nash on 16 January 2023, 14:35 > RESOLVED. ---------------------------------------- File: cloudsync/utils/reachability.py Revision Comment by Behrouz NematiPour on 12 December 2022, 11:49 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15189 Please don't use google domain for any test. Please use the defined knowns used URLs for the DCS, since if they are not working internet connection doesn't mean and help with anything. Reply by Behrouz NematiPour on 13 January 2023, 16:47 > Radu: > "Fixed" Reply by Behrouz NematiPour on 13 January 2023, 16:47 > RESOLVED Revision Comment by Behrouz NematiPour on 12 December 2022, 11:50 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15190 Please don't use any magic numbers and define a variable for each to give it a meaning and definition. Reply by Behrouz NematiPour on 13 January 2023, 16:47 > Radu: > "Fixed" Reply by Behrouz NematiPour on 13 January 2023, 16:48 > REOLVED Reply by Dara Navaei on 19 October 2023, 10:37 > RESOLVED ---------------------------------------- File: cs.py Revision Comment by Behrouz NematiPour on 12 December 2022, 11:34 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15186 This file needs to be auto executed bash sscript by adding #!/usr/bin/python3 and do the: chmod a+x on this file Reply by Behrouz NematiPour on 13 January 2023, 16:49 > Radu: > "Fixed" Reply by Behrouz NematiPour on 13 January 2023, 16:49 > RESOLVED Revision Comment by Sean Nash on 14 December 2022, 11:03 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15214 Magic number. Reply by Behrouz NematiPour on 13 January 2023, 16:49 > Radu: > "Fixed" Reply by Sean Nash on 16 January 2023, 14:36 > RESOLVED. Revision Comment by Sean Nash on 14 December 2022, 11:02 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1#c15213 Argument numbers (magic numbers) could be clarified with well named constants. Reply by Behrouz NematiPour on 13 January 2023, 16:52 > Radu: > "Fixed" Reply by Sean Nash on 16 January 2023, 14:36 > RESOLVED. --- ID: CS-BUILD-0-2-4-1 https://devapps.diality.us/cru/CS-BUILD-0-2-4-1 Title: CS-Build-0.2.4 Statement of Objectives: State: Closed Summary: Author: Behrouz NematiPour Reviewers: (5 active, 0 completed*) Sean Nash Tiffany Mejia Michael Garthwaite Dara Navaei jishii