This is a list of all comments for DIALIN-DEN-4690-1. Review Summary: No summary ---------------------------------------- File: dialin/squish/crc.py Revision Comment by pmontazemi on 08 September 2020, 09:56 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c4418 Remove second copyright. Reply by Behrouz NematiPour on 08 September 2020, 10:58 > Removed. Reply by pmontazemi on 09 September 2020, 14:45 > RESOLVED. Revision Comment by plucia on 07 October 2020, 13:34 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c5351 Please add the return type Reply by Behrouz NematiPour on 07 October 2020, 14:59 > Added Reply by plucia on 14 October 2020, 10:21 > RESOLVED Revision Comment by plucia on 07 October 2020, 13:35 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c5352 Please add the return type and fill in the parameter descriptions Reply by Behrouz NematiPour on 07 October 2020, 14:59 > Added Reply by plucia on 14 October 2020, 10:21 > RESOLVED ---------------------------------------- File: dialin/squish/denaliMessages.py Revision Comment by plucia on 30 September 2020, 15:27 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c5037 This needs to be updated to use the DenaliCanMessenger instead of cansend Reply by Behrouz NematiPour on 03 October 2020, 21:03 > "*** Off the subject ***" > Thanks for reminding that. > This is exactly what I've told and mentioned while ago at the > beginning of adding these methods as part of Dialin, that was > the main reason we did it. > As we talked while ago and you're the Dialin owner, please > create a story for yourself and cooperate with our manager to > define the priority/plan for the story. > From that moment on, all the newly added methods shall follow > that design and implementation. Reply by plucia on 14 October 2020, 10:18 > Will be tracked in DIAL-41 > RESOLVED. Revision Comment by plucia on 07 October 2020, 13:36 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c5353 The return types are missing for many functions in this file Reply by Behrouz NematiPour on 07 October 2020, 14:39 > I rechecked, > All the functions have :return: > What function you mean? Reply by plucia on 14 October 2020, 10:17 > RESOLVED Revision Comment by plucia on 05 October 2020, 17:13 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c5250 Need to add the types for each parameter as well to be consistent with the other dialin classes Reply by Behrouz NematiPour on 06 October 2020, 12:21 > Added Reply by plucia on 07 October 2020, 13:31 > RESOLVED Revision Comment by plucia on 30 September 2020, 15:21 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c5035 There should be a description in words describing what this function does. There should be a type for each parameter and a description of the parameter. Right now the documentation is missing: X:/Users/Bamboo-Projects/DEN_DX/Dialin/Doxument-master/html/dialin.squish.html Have you tested the frame diagram with sphinx? Reply by Behrouz NematiPour on 05 October 2020, 10:37 > Description added. > I am not familiar with Sphinx and that's not the > documentation we should use for UI Software. > The comments are in the Markup language and Sphinx, like all > the other documentation tools, must have a way to understand > it. Reply by pmontazemi on 06 October 2020, 09:48 > [~bnematipour] FYI, since the beginning of Dialin API when > PeterL took over its product ownership, we made the > decision to use Sphinx for its documentation because it is > more suited for Python-based code. And, although Dialin API > interacts with UI APP and DG/HD FW, it is a complete > separate package of its own, which will never end in the > final product, hence its categorization as non-product > software. Reply by Behrouz NematiPour on 06 October 2020, 12:22 > Thanks Peman for clarification. > As discussed with Peter, updated the docstrings. Reply by plucia on 14 October 2020, 10:17 > RESOLVED Revision Comment by pmontazemi on 05 October 2020, 15:50 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c5211 Ultrafiltionation? Reply by Behrouz NematiPour on 06 October 2020, 12:33 > fixed. Reply by pmontazemi on 07 October 2020, 10:37 > RESOLVED. Revision Comment by pmontazemi on 05 October 2020, 15:50 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c5212 Misspelling in all three lines for word ultrafiltration Reply by Behrouz NematiPour on 06 October 2020, 12:32 > fixed Reply by pmontazemi on 07 October 2020, 10:36 > RESOLVED. Revision Comment by pmontazemi on 05 October 2020, 15:51 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c5213 Misspelling in all three lines for word ultrafiltration Reply by Behrouz NematiPour on 06 October 2020, 12:32 > fixed Reply by pmontazemi on 07 October 2020, 10:36 > RESOLVED. Revision Comment by plucia on 09 September 2020, 13:27 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c4583 Description is missing for the parameters here and many of the other functions Reply by Behrouz NematiPour on 09 September 2020, 17:30 > These are all the FW specific values and terms I don't know > about them much and not all of them are used. > So I don't know the specific definition for them to put them. > We need to ask the FW team to provide us more descriptive > definitions than the one in the Messaging List. > Or dig deep into the FW SDD to find them out. Reply by plucia on 30 September 2020, 10:16 > This file is also not PEP8 compliant. > Please, documentation for all functions needs to be added Reply by Behrouz NematiPour on 30 September 2020, 11:49 > I put comments on every function. > Is there a function that doesn't have a comment? > As I mentioned there are some parameters that UI doesn't > use and doesn't know the description. Reply by plucia on 30 September 2020, 15:20 > The docstring needs to contain a description of the > function and the type of each parameter as well as a > description for each parameter. Reply by Behrouz NematiPour on 05 October 2020, 10:40 > "*** Please don't check "Needs Resolution" for each > comment you made ***" > Added Reply by plucia on 14 October 2020, 10:27 > RESOLVED. Revision Comment by plucia on 30 September 2020, 15:18 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c5032 A description is needed here in words of what this function does Reply by Behrouz NematiPour on 05 October 2020, 10:40 > Added Reply by plucia on 14 October 2020, 10:18 > RESOLVED ---------------------------------------- File: dialin/squish/messageBuilder.py Revision Comment by plucia on 07 October 2020, 13:31 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c5342 This should be removed Reply by Behrouz NematiPour on 07 October 2020, 17:08 > "*** Off the subject ***" > This code review is about the documentation, not the code > change. > There is no implementation and unit testing associated with > this story. Reply by plucia on 14 October 2020, 10:26 > Fair, not part of story. > > RESOLVED Revision Comment by plucia on 07 October 2020, 13:31 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c5341 This should be removed Reply by Behrouz NematiPour on 07 October 2020, 17:08 > "*** Off the subject ***" > This code review is about the documentation, not the code > change. > There is no implementation and unit testing associated with > this story. Reply by plucia on 14 October 2020, 10:26 > RESOLVED. > > Fair, not part of story ---------------------------------------- File: dialin/squish/unittests.py Revision Comment by pmontazemi on 08 September 2020, 09:56 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c4417 Remove second copyright. Reply by Behrouz NematiPour on 08 September 2020, 10:58 > Removed. Reply by pmontazemi on 09 September 2020, 14:47 > RESOLVED. Revision Comment by plucia on 07 October 2020, 13:31 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c5343 This should be removed Reply by Behrouz NematiPour on 07 October 2020, 17:07 > "*** Off the subject ***" > This code review is about the documentation, not the code > change. > There is no implementation and unit testing associated with > this story. Reply by plucia on 14 October 2020, 10:24 > RESOLVED. > > Fair, not part of story. Revision Comment by plucia on 07 October 2020, 13:32 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c5344 This should be removed Reply by Behrouz NematiPour on 07 October 2020, 17:08 > "*** Off the subject ***" > This code review is about the documentation, not the code > change. > There is no implementation and unit testing associated with > this story. Reply by plucia on 14 October 2020, 10:24 > RESOLVED. > > Fair, not part of story. ---------------------------------------- File: dialin/squish/utils.py Revision Comment by pmontazemi on 08 September 2020, 09:56 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c4419 Remove second copyright. Reply by Behrouz NematiPour on 08 September 2020, 10:58 > Removed. Reply by pmontazemi on 09 September 2020, 14:47 > RESOLVED. Revision Comment by plucia on 07 October 2020, 13:32 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c5345 This function name is not clear. What does it do? Reply by Behrouz NematiPour on 07 October 2020, 16:33 > added Reply by plucia on 14 October 2020, 10:26 > RESOLVED. Revision Comment by plucia on 07 October 2020, 13:32 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c5346 This function name could be clearer Reply by Behrouz NematiPour on 07 October 2020, 16:34 > "*** Almost off the subject ***" > the function is doing partitioning the string. > added description. Reply by plucia on 14 October 2020, 10:24 > RESOLVED Revision Comment by plucia on 07 October 2020, 13:33 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c5347 This function name could be clearer Please remove the v prefix. It doesn't add any information Reply by Behrouz NematiPour on 07 October 2020, 16:36 > "*** almost off the subject ***" > it makes the parameter to be identified as a value, not a > type. > there is no preference for function argument naming in python > coding style. Reply by plucia on 14 October 2020, 10:23 > RESOLVED Revision Comment by plucia on 07 October 2020, 13:33 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c5348 You're missing the return type Reply by Behrouz NematiPour on 07 October 2020, 17:03 > Added Reply by plucia on 14 October 2020, 10:23 > RESOLVED Revision Comment by plucia on 07 October 2020, 13:33 https://devapps.diality.us/cru/DIALIN-DEN-4690-1#c5349 You're missing the return type, please fix all other missing return types Reply by Behrouz NematiPour on 07 October 2020, 17:03 > Added Reply by plucia on 14 October 2020, 10:22 > RESOLVED --- ID: DIALIN-DEN-4690-1 https://devapps.diality.us/cru/DIALIN-DEN-4690-1 Title: DIALIN-DEN-4690_ Doxygenization Statement of Objectives: State: Closed Summary: Author: Behrouz NematiPour Moderator: Behrouz NematiPour Reviewers: (0 active, 2 completed*) plucia (*) pmontazemi (*)