fix(rhino): CNX-8388 handle ca1031 warnings in rhino connector #3110

clairekuang · 2023-12-14T13:24:59Z

Description & motivation

Resolves ca1031 exception handling warnings in the rhino connector, where easily done.
Includes minor refactor and improvement for layer handling methods in Utils after finding bugs during testing.
Adds comments for unresolved ca1031 warnings, to be investigated further.

Changes:

-Rhino connector

…ects

AlanRynne

Added some comments so I won't forget when we do our live review @clairekuang.

Also pinged @JR-Morgan and @BovineOx in a potentially interesting one.

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/Entry/Plugin.cs

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/ConnectorBindingsRhino.Previews.cs

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/ConnectorBindingsRhino.Receive.cs

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/ConnectorBindingsRhino.Selection.cs

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/MappingBindingsRhino.cs

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/Utils.cs

BovineOx

I'll leave my review there as I think we are learning that we need to pool the exception experience and provide more guidance. We also need a pattern that involves a ticket for following up so we don't feel compelled to write more todos into the code :)

If TODOs are also not a warning...

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/Entry/Plugin.cs

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/ConnectorBindingsRhino.Previews.cs

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/ConnectorBindingsRhino.Receive.cs

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/ConnectorBindingsRhino.Selection.cs

BovineOx · 2023-12-14T21:21:58Z

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/MappingBindingsRhino.cs

@@ -26,41 +26,44 @@ public MappingBindingsRhino()

  public override MappingSelectionInfo GetSelectionInfo()
  {
-    try
+    List<RhinoObject> selection = RhinoDoc.ActiveDoc.Objects.GetSelectedObjects(false, false)?.ToList();


legitimate I assume here to, that we expect each of these references to not be null?

Can confirm that .Objects should never return null, but it's definitely not great to use ActiveDoc as per rhino documentation: WARNING!! Do not use the ActiveDoc if you don't have to. Under Mac Rhino the ActiveDoc can change while a command is running. Use the doc that is passed to you in your RunCommand function or continue to use the same doc after the first call to ActiveDoc.

This it out of scope for this ticket however so I'm leaving it as is unless @AlanRynne knows if the call to ActiveDoc would ever return null (eg when called from rhino.inside?)

It certainly feels like connector/converter logic requires a context object which would contain the document in context for the current calling context... I need to play around with this in autofac so we can embody this in some DI in the future

ActiveDoc can and will be null when running on Rhino.Compute, among other scenarios. But since rhino itself cannot be run headless (only grasshopper can AFAIK) this is a non-issue in the Rhino connector.

The need for a context object is real, there is contextual information that the connector needs to pass to the converter for it to do it's work properly (document, conversion caches, etc...) and some sub-conversions require different contextual information (i.e. Units.None instead of document units) to do their work properly.

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/MappingBindingsRhino.cs

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/Entry/Plugin.cs

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/Entry/SpeckleMappingsCommandWin.cs

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/MappingBindingsRhino.cs

…ects

…in converter

BovineOx

Apologies I think I added more comments only now I see them as pending 😨

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/Utils.cs

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/Entry/Plugin.cs

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/Entry/SpeckleCommandWin.cs

BovineOx · 2023-12-20T10:08:30Z

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/MappingBindingsRhino.cs

-        {
-          return new MappingSelectionInfo(new List<Schema>(), selection.Count);
-        }
+        catch (ArgumentNullException) { }


Not sure why this would fail and I would like to understand what the consequences are and have the failure recorded in the report.

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/MappingBindingsRhino.cs

BovineOx · 2023-12-20T16:09:39Z

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/MappingBindingsRhino.cs

@@ -26,41 +26,44 @@ public MappingBindingsRhino()

  public override MappingSelectionInfo GetSelectionInfo()
  {
-    try
+    List<RhinoObject> selection = RhinoDoc.ActiveDoc.Objects.GetSelectedObjects(false, false)?.ToList();


It certainly feels like connector/converter logic requires a context object which would contain the document in context for the current calling context... I need to play around with this in autofac so we can embody this in some DI in the future

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/MappingBindingsRhino.cs

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/Utils.cs

…ects

AlanRynne

Fixed most of the pending comments from @BovineOx except 2

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/MappingBindingsRhino.cs

…ects

AlanRynne

Approving after chat with Claire last night

If anything else pops up, we can deal with it in future tickets.

clairekuang added 2 commits December 14, 2023 12:31

fixes exception handling in rhino connector files

3c5a262

refactors getlayer method to be more robust

bd264bb

clairekuang requested a review from AlanRynne December 14, 2023 13:24

clairekuang added 4 commits December 14, 2023 13:25

Merge branch 'dev' into CNX-8388-Handle-CA1031-warnings-in-Rhino-proj…

4440db5

…ects

refactors to use new GetGuidFromString method

91c7277

removes unnecessary else blocks

25b2010

fixes one more ca1031 warning

8da95db

clairekuang requested a review from BovineOx December 14, 2023 14:03

AlanRynne reviewed Dec 14, 2023

View reviewed changes

BovineOx reviewed Dec 14, 2023

View reviewed changes

AlanRynne changed the title ~~fix(rhino): Cnx 8388 handle ca1031 warnings in rhino connector~~ fix(rhino): CNX-8388 handle ca1031 warnings in rhino connector Dec 19, 2023

BovineOx reviewed Dec 20, 2023

View reviewed changes

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/Entry/Plugin.cs Outdated Show resolved Hide resolved

BovineOx reviewed Dec 20, 2023

View reviewed changes

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/Entry/SpeckleMappingsCommandWin.cs Show resolved Hide resolved

BovineOx reviewed Dec 20, 2023

View reviewed changes

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/MappingBindingsRhino.cs Outdated Show resolved Hide resolved

BovineOx reviewed Dec 20, 2023

View reviewed changes

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/MappingBindingsRhino.cs Outdated Show resolved Hide resolved

BovineOx reviewed Dec 20, 2023

View reviewed changes

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/MappingBindingsRhino.cs Show resolved Hide resolved

clairekuang added 6 commits December 20, 2023 10:59

Merge branch 'dev' into CNX-8388-Handle-CA1031-warnings-in-Rhino-proj…

4367f51

…ects

pr fixes

78143e4

Update MappingBindingsRhino.cs

f6a8edd

Update Utils.cs

b368bf5

adds conversionnotsupported exceptions and better exception handling …

d7e4d2e

…in converter

Update ConverterRhinoGh.cs

959f03a

BovineOx reviewed Dec 20, 2023

View reviewed changes

removes unnecessary disposal

716f8f6

BovineOx reviewed Jan 2, 2024

View reviewed changes

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/MappingBindingsRhino.cs Show resolved Hide resolved

BovineOx reviewed Jan 2, 2024

View reviewed changes

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/MappingBindingsRhino.cs Outdated Show resolved Hide resolved

BovineOx reviewed Jan 2, 2024

View reviewed changes

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/MappingBindingsRhino.cs Show resolved Hide resolved

BovineOx reviewed Jan 2, 2024

View reviewed changes

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/Utils.cs Outdated Show resolved Hide resolved

AlanRynne added 2 commits January 8, 2024 12:46

Merge branch 'dev' into CNX-8388-Handle-CA1031-warnings-in-Rhino-proj…

03ac9d3

…ects

fix(rhino): Added use of IsFatal in converter

1d175f2

AlanRynne added 3 commits January 8, 2024 17:03

fix(rhino): Added use of IsFatal in connector

58b586b

fix(rhino): Missing newline

5bad3f3

fix(rhino): Wrapped all generic exceptions with isFatal handling

b53dc36

AlanRynne reviewed Jan 8, 2024

View reviewed changes

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/MappingBindingsRhino.cs Outdated Show resolved Hide resolved

ConnectorRhino/ConnectorRhino/ConnectorRhinoShared/UI/MappingBindingsRhino.cs Show resolved Hide resolved

fix(rhino): Typo in isFatal handling

4f3de03

AlanRynne self-requested a review January 8, 2024 16:45

Merge branch 'dev' into CNX-8388-Handle-CA1031-warnings-in-Rhino-proj…

e482453

…ects

clairekuang requested a review from BovineOx January 8, 2024 18:22

AlanRynne approved these changes Jan 9, 2024

View reviewed changes

AlanRynne merged commit 5bf6a8d into dev Jan 9, 2024
30 checks passed

AlanRynne deleted the CNX-8388-Handle-CA1031-warnings-in-Rhino-projects branch January 9, 2024 09:04

JR-Morgan added this to the 2.18 milestone Jan 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rhino): CNX-8388 handle ca1031 warnings in rhino connector #3110

fix(rhino): CNX-8388 handle ca1031 warnings in rhino connector #3110

clairekuang commented Dec 14, 2023

AlanRynne left a comment

BovineOx left a comment

BovineOx Dec 14, 2023

clairekuang Dec 20, 2023

BovineOx Dec 20, 2023

AlanRynne Jan 8, 2024 •

edited

BovineOx left a comment

BovineOx Dec 20, 2023

BovineOx Dec 20, 2023

AlanRynne left a comment

AlanRynne left a comment

fix(rhino): CNX-8388 handle ca1031 warnings in rhino connector #3110

fix(rhino): CNX-8388 handle ca1031 warnings in rhino connector #3110

Conversation

clairekuang commented Dec 14, 2023

Description & motivation

Changes:

AlanRynne left a comment

Choose a reason for hiding this comment

BovineOx left a comment

Choose a reason for hiding this comment

BovineOx Dec 14, 2023

Choose a reason for hiding this comment

clairekuang Dec 20, 2023

Choose a reason for hiding this comment

BovineOx Dec 20, 2023

Choose a reason for hiding this comment

AlanRynne Jan 8, 2024 • edited

Choose a reason for hiding this comment

BovineOx left a comment

Choose a reason for hiding this comment

BovineOx Dec 20, 2023

Choose a reason for hiding this comment

BovineOx Dec 20, 2023

Choose a reason for hiding this comment

AlanRynne left a comment

Choose a reason for hiding this comment

AlanRynne left a comment

Choose a reason for hiding this comment

AlanRynne Jan 8, 2024 •

edited