This is a list of all comments for DIALIN-DEN-8308-1.

Review Summary: No summary


----------------------------------------
File: dialin/ui/hd_simulator_alarms.py
Revision Comment by pmontazemi on 07 May 2021, 10:23 
https://devapps.diality.us/cru/DIALIN-DEN-8308-1#c9801

  What information is needed to replace these TBDs?

  Reply by Behrouz NematiPour on 18 May 2021, 21:15
  > I'm not aware of this function usage, but in general, some of
  > these fields provided by HD for later use, and UI is not
  > using them and some other has recently been used.

    Reply by pmontazemi on 19 May 2021, 15:20
    > RESOLVED.

----------------------------------------
File: dialin/ui/hd_simulator.py
Revision Comment by pmontazemi on 17 May 2021, 09:19 
https://devapps.diality.us/cru/DIALIN-DEN-8308-1#c9868

  Add space after comma right before hasParameters.

  Reply by Behrouz NematiPour on 18 May 2021, 21:14
  > Updated

    Reply by pmontazemi on 19 May 2021, 15:20
    > RESOLVED.

Revision Comment by plucia on 18 May 2021, 16:13 
https://devapps.diality.us/cru/DIALIN-DEN-8308-1#c9934

  Please use snake_case for all variables
  Typing information is missing for parameters
  Typing information is missing for the return type

  Reply by Behrouz NematiPour on 18 May 2021, 21:14
  > Updated

    Reply by plucia on 28 May 2021, 17:15
    > RESOLVED

Revision Comment by plucia on 24 May 2021, 07:31 
https://devapps.diality.us/cru/DIALIN-DEN-8308-1#c10042

  I see a couple of issues here:
  The message response logic has been moved out of dialin and
  into your UI simulator in the testsuites respository. This
  creates a couple of problems: First, it upends the entire
  design of the HD / DG simulator. Second, it splits the
  responsibility for simulating an HD and DG into both dialin and
  your UI simulator program. This means that the caller of the
  dialin command must know how to construct a response. If I want
  to use your functions to simulate a response, I can't because
  they don't exist here. The logic for simulating an HD and DG
  must be fully contained inside the HD and DG simulators so
  other users of the API can call it, not dispersed to outside
  scripts. Also, your changes appear to be on an extremely old
  version of dialin. These changes are incompatible with the
  latest changes on staging. Please merge staging into your
  branch.
  
  - A message ID must not be passed to a simulator command. This
  ensures there is a one to one mapping between dialin commands
  and messages and that dialin can do its job of abstracting
  messages. This is how dialin works everywhere else and should
  not be changed.
  - Parameters should not be pre-converted to bytearrays before
  being sent to dialin.
  - If you have one, two, or five parameters in a denali message,
  your dialin function should accept one, two, or five
  parameters. This ensures the payload is clearly defined and
  described by the number of parameters passed to the function
  and that the function can do its job of abstracting the denali
  message.

  Reply by Behrouz NematiPour on 24 May 2021, 23:45
  > Updated regarding our conversation.

    Reply by plucia on 28 May 2021, 17:15
    > RESOLVED

Revision Comment by plucia on 28 May 2021, 17:15 
https://devapps.diality.us/cru/DIALIN-DEN-8308-1#c10147

  The typing information is missing for accepted and reason here

  Reply by Behrouz NematiPour on 28 May 2021, 18:04
  > Has been addressed in
  > http://dvm-linux02:8060/cru/DIALIN-DEN-8514-1

    Reply by plucia on 01 June 2021, 10:46
    > RESOLVED

Revision Comment by plucia on 28 May 2021, 17:17 
https://devapps.diality.us/cru/DIALIN-DEN-8308-1#c10151

  Should indicate the return type:
  instead of 
  {code}): {code} 
  it should be
   {code}) -> None:{code}

  Reply by Behrouz NematiPour on 28 May 2021, 18:02
  > Has been addressed in
  > http://dvm-linux02:8060/cru/DIALIN-DEN-8514-1

    Reply by plucia on 01 June 2021, 10:44
    > RESOLVED

Revision Comment by plucia on 28 May 2021, 17:15 
https://devapps.diality.us/cru/DIALIN-DEN-8308-1#c10148

  Missing the cmd_ prefix in the function name

  Reply by Behrouz NematiPour on 28 May 2021, 18:03
  > Has been addressed in
  > http://dvm-linux02:8060/cru/DIALIN-DEN-8514-1

    Reply by plucia on 01 June 2021, 10:45
    > RESOLVED

Revision Comment by plucia on 28 May 2021, 17:16 
https://devapps.diality.us/cru/DIALIN-DEN-8308-1#c10149

  Missing the cmd_ prefix in the function name

  Reply by Behrouz NematiPour on 28 May 2021, 18:03
  > Has been addressed in
  > http://dvm-linux02:8060/cru/DIALIN-DEN-8514-1

    Reply by plucia on 01 June 2021, 10:45
    > RESOLVED


---

ID: DIALIN-DEN-8308-1 https://devapps.diality.us/cru/DIALIN-DEN-8308-1

Title: DIALIN-DEN-8308_UI DEV Treatment Log
Statement of Objectives:
* DEN-8316: TxLog: Implementation [Simulator/Testing]

State: Closed

Summary: 

Author: Behrouz NematiPour
Moderator: Behrouz NematiPour
Reviewers: (0 active, 2 completed*)
     plucia   (*)
     pmontazemi   (*)