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.

Sign up for GitHub

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

Jump to bottom

Fix(Revit) : CNX-8387 ca1031 don't catch general exceptions #3106

Merged
merged 18 commits into from
Jan 10, 2024
Merged

Fix(Revit) : CNX-8387 ca1031 don't catch general exceptions #3106

merged 18 commits into from
Jan 10, 2024

Conversation

connorivy
Copy link
Contributor

Description & motivation

Changes:

  • when it was obvious which exception would be thrown, catch statement was altered to that exception
  • when it was not obvious, seq logs were checked
    • if exception was being logged and had not occured in the last few months, the try-catch was removed
    • if exception was not being logged, seq logging was added

To-do before merge:

Screenshots:

Validation of changes:

Checklist:

  • My pull request follows the guidelines in the Contributing guide?
  • My pull request does not duplicate any other open Pull Requests for the same update/change?
  • My commits are related to the pull request and do not amend unrelated code or documentation.
  • My code follows a similar style to existing code.
  • I have added appropriate tests.
  • I have updated or added relevant documentation.

References

@JR-Morgan JR-Morgan added this to the 2.18 milestone Dec 13, 2023
@JR-Morgan JR-Morgan added revit issues related to the revit connector. tech debt labels Dec 13, 2023
AlanRynne
AlanRynne suggested changes Dec 14, 2023
View reviewed changes
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.

I guess there's still a lot we need to figure out, and since your PR is the first... we're hitting a bunch of existential questions.

Only half way there but I'm taking a break for the day and didn't want to loose this. I'll do a review of the other half tomorrow

BovineOx
BovineOx reviewed Dec 14, 2023
View reviewed changes
Copy link
Contributor

@BovineOx BovineOx left a comment

Choose a reason for hiding this comment

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

I think we need to provide a bit more guidance @connorivy so I'll Leave it there :)

BovineOx
BovineOx reviewed Dec 20, 2023
View reviewed changes
ConnectorRevit/ConnectorRevit/Entry/App.cs Outdated
{
SpeckleLog.Logger.Warning(ex, "Error loading kit on startup");
NotifyUserOfErrorStartingConnector(ex);
}
catch (Exception ex)
Copy link
Contributor

Choose a reason for hiding this comment

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

just reference CNX-8644 here

BovineOx
BovineOx reviewed Dec 20, 2023
View reviewed changes
Objects/Converters/ConverterRevit/ConverterRevitTests/ConverterRevitTestsShared/SpeckleUtils.cs
@@ -46,6 +47,7 @@ internal static class SpeckleUtils
{
tcs.TrySetException(exception);
}
#pragma warning restore CA1031 // Do not catch general exception types
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about this one, possible we consider it a TL handler because of what it is? Not convinced though

Copy link
Member

Choose a reason for hiding this comment

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

Assuming we ensure the task completion source is always observed. I think its safe to catch all here.
However, to be extra safe. In the few places of Core where we are using TaskCompletionSources, I'm using this pattern

https://github.com/specklesystems/speckle-sharp/blob/d0952efb1965bbd75e64ddebd828e86c368b2290/Core/Core/Serialisation/DeserializationWorkerThreads.cs#L51-L63C6

we may consider doing something similar here.

Copy link
Member

Choose a reason for hiding this comment

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

That being said.. I'm a bit puzzled why we're using a Task completion source here anyway. since we await the APIContext call anyway...

@AlanRynne AlanRynne changed the title Fix(Revit) : ca1031 don't catch general exceptions Fix(Revit) : CNX-8387 ca1031 don't catch general exceptions Jan 8, 2024
AlanRynne
AlanRynne approved these changes Jan 8, 2024
View reviewed changes
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.

Approving with some minor comments!

AlanRynne
AlanRynne approved these changes Jan 9, 2024
View reviewed changes
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.

All the last proposed changes look good. I'm merging this in.

@AlanRynne AlanRynne self-assigned this Jan 10, 2024
@AlanRynne AlanRynne merged commit 22a4cac into dev Jan 10, 2024
30 checks passed
@AlanRynne AlanRynne deleted the revit/connor/ca1031 branch January 10, 2024 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlanRynne AlanRynne AlanRynne approved these changes

@JR-Morgan JR-Morgan JR-Morgan left review comments

@BovineOx BovineOx BovineOx left review comments

Assignees

@AlanRynne AlanRynne

Labels
revit issues related to the revit connector. tech debt
Projects
None yet
Milestone
2.18
Development

Successfully merging this pull request may close these issues.
4 participants
@connorivy @AlanRynne @JR-Morgan @BovineOx