Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(autocadcivil): CNX-8389 fixes CA1031 warnings in autocad and civil3d projects #3119

Merged
merged 16 commits into from
Jan 12, 2024

Conversation

clairekuang
Copy link
Member

@clairekuang clairekuang commented Dec 21, 2023

Description & motivation

Fixes CA1031 in Autocad and Civil3D.

Changes:

  • Autocad and Civil3d Connectors and Converters

To-do before merge:

  • Check for Speckle Logging initialization in Connector
  • Replace !IsFatal() TODOS

@@ -1142,6 +1052,7 @@ public Pipe PipeToSpeckle(CivilDB.Pipe pipe)
// assign additional pipe props
AddNameAndDescriptionProperty(pipe.Name, pipe.Description, specklePipe);

// TODO: use !IsFatal() here
Copy link
Contributor

@BovineOx BovineOx Jan 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ouch, what is throwing here? Is it the ToString() or can some things be null? Having this many try/catch blocks smells a bit, is there nothing we can do to avoid this? I might be more inclined to call a method that wraps the try/catch if it's unavoidable, unless this is likely to add performance overhead?

Copy link
Member Author

@clairekuang clairekuang Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accessing some properties on the pipe object may throw for undocumented reasons (usually if the property doesn't exist or isn't applicable). It's worth trying to add as many as possible to the pipe, not knowing which may throw, thus the individual try catches.

Leaving this as is since I can't think of a better way to handle it

Copy link
Contributor

@AlanRynne AlanRynne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting this to "requested changes". @clairekuang is already working through them and I'll re-review once they're in

@AlanRynne AlanRynne changed the title refactor(autocadcivil): fixes CA1031 warnings in autocad and civil3d projects refactor(autocadcivil): CNX-8389 fixes CA1031 warnings in autocad and civil3d projects Jan 10, 2024
@AlanRynne AlanRynne self-assigned this Jan 10, 2024
Copy link
Contributor

@AlanRynne AlanRynne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly good. Just some missing IsFatal() calls, a typo and a missing log call 👍🏻

@clairekuang clairekuang requested a review from AlanRynne January 12, 2024 13:03
Copy link
Contributor

@AlanRynne AlanRynne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great now 🙌🏼

@AlanRynne AlanRynne merged commit 3db9590 into dev Jan 12, 2024
30 checks passed
@AlanRynne AlanRynne deleted the CNX-8389-Handle-CA1031-warnings-in-AutoCAD-projects branch January 12, 2024 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants