Dynamo icon indicating copy to clipboard operation
Dynamo copied to clipboard

Return object instead of IList from ReplicationLogic

Open aparajit-pratap opened this issue 2 years ago • 2 comments

P### Purpose

Return object instead of IList from ReplicationLogic

Hopefully this should address https://jira.autodesk.com/browse/DYN-5152

Declarations

Check these if you believe they are true

  • [x] The codebase is in a better state after this PR
  • [ ] Is documented according to the standards
  • [ ] The level of testing this PR includes is appropriate
  • [ ] User facing strings, if any, are extracted into *.resx files
  • [ ] All tests pass using the self-service CI.
  • [ ] Snapshot of UI changes, if any.
  • [ ] Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • [ ] This PR modifies some build requirements and the readme is updated

Release Notes

(FILL ME IN) Brief description of the fix / enhancement. Mandatory section

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

aparajit-pratap avatar Sep 20 '22 14:09 aparajit-pratap

@aparajit-pratap Looks like you simply replace the IList with object without any regressions (asides from mending a few tests) I guess I am now thinking why we needed the IList in the first place ...

pinzart90 avatar Sep 21 '22 14:09 pinzart90

@aparajit-pratap Looks like you simply replace the IList with object without any regressions (asides from mending a few tests) I guess I am now thinking why we needed the IList in the first place ...

@pinzart90 I also made a few changes in the replication code. PTAL.

I guess choosing IList was the wrong choice. I know better now :)

aparajit-pratap avatar Sep 21 '22 16:09 aparajit-pratap

I think theres something to be gained by profiling and investigating ways to improve the performance of collection marshaling - but that seems like it's out of scope for this PR.

mjkkirschner avatar Sep 27 '22 17:09 mjkkirschner