fix:(tekla): CNX-8391 Handle warnings CA1031 #3118
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
continues on the specific error previously commented that should be swallowed, but throws on all other insertion errors.
…ures The code changes add logging functionality to handle exceptions thrown by the DynamicBase class when setting properties that cannot be set. The exceptions are not fatal and should simply be logged. The commit also includes a warning message with details about the element, property, and reason for skipping.
This commit adds the following changes to the MainForm class: - Added an InitializeComponents method to initialize the components of the MainForm. - Added a SetupApplication method to set up the application, including event subscriptions, UI build, and model bindings. - Added a SubscribeToEvents method to subscribe to necessary application events. - Added a BuildAndShowMainWindow method to build and display the main window of the application. - Added a SetTeklaAsOwner method to set the Tekla application as the owner of the main window. These changes improve the initialization and setup process in MainForm, ensuring that necessary components are properly initialized and that event subscriptions, UI build, and model bindings are correctly set up.
This commit adds logging and error handling to the StreamStateManager class. It introduces the use of Speckle.Core.Logging for logging errors. Significant changes include: - Added a using statement for the System namespace - Added a using statement for the Speckle.Core.Logging namespace - Implemented error handling in ReadState method, catching JsonException and returning an empty list if deserialization fails - Implemented error handling in WriteState method, catching exceptions and logging an error message if writing to file fails - Implemented error handling in ClearStreamStateList method, catching exceptions and logging an error message if clearing the stream state file fails - Updated comments with more detailed explanations of methods and their behavior
BovineOx
reviewed
Dec 20, 2023
ConnectorTeklaStructures/ConnectorTeklaStructuresShared/MainForm.cs
Outdated
Show resolved
Hide resolved
...ectorTeklaStructures/ConnectorTeklaStructuresShared/StreamStateManager/StreamStateManager.cs
Show resolved
Hide resolved
...ectorTeklaStructures/ConnectorTeklaStructuresShared/StreamStateManager/StreamStateManager.cs
Outdated
Show resolved
Hide resolved
...ectorTeklaStructures/ConnectorTeklaStructuresShared/StreamStateManager/StreamStateManager.cs
Show resolved
Hide resolved
...erterTeklaStructures/ConverterTeklaStructuresShared/PartialClasses/ConvertDirectShapeMesh.cs
Show resolved
Hide resolved
- Renamed private field `_speckleFilePath` to `s_speckleFilePath` - Added logging warning when deserialization of saved stream state list fails - Updated method `WriteStreamStateList` to handle empty or null stream state string - Updated method `ClearStreamStateList` to handle null or non-existent speckle file path - Updated method `GetOrCreateSpeckleFilePath` to use lowercase variable names and added error handling for file creation/retrieval - Updated method `ReadSpeckleFile` to use lowercase variable name and added error handling for file reading
BovineOx
reviewed
Jan 4, 2024
...ectorTeklaStructures/ConnectorTeklaStructuresShared/StreamStateManager/StreamStateManager.cs
Outdated
Show resolved
Hide resolved
BovineOx
reviewed
Jan 4, 2024
...ectorTeklaStructures/ConnectorTeklaStructuresShared/StreamStateManager/StreamStateManager.cs
Outdated
Show resolved
Hide resolved
BovineOx
reviewed
Jan 4, 2024
...erterTeklaStructures/ConverterTeklaStructuresShared/PartialClasses/ConvertDirectShapeMesh.cs
Show resolved
Hide resolved
|
Just a few things to consider, but minor, once you've done so happy for you to resolve the comments - ping me if it needs an approval still |
Previously, there was a TODO comment suggesting to consider what to do in the case where the insert operation returned false but didn't throw an exception and no matching shape was found. This commit changes that behavior by throwing a `ConversionException` with an appropriate error message instead.
The code changes fix an issue where the file stream was not being properly checked for write access before attempting to write to it. This could result in an error when trying to write the stream state list to the file. The changes include adding a check for write access and throwing a `SpeckleException` with an appropriate error message if writing fails.
The commit fixes the exception handling in the SetupApplication method. Instead of catching all exceptions and logging them, it now catches only general exceptions. The error message is also updated to include the specific error message from the caught exception.
AlanRynne
approved these changes
Jan 8, 2024
AlanRynne
deleted the
CNX-8391-Handle-CA1031-warnings-in-Tekla-projects
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
THIS PR SHOULD BE MERGED AFTER ExceptionHelpers has merged.
Changes to:
Connector
MainForm
StreamStateManager
Converter
ConverterTeklaStructureUtils
ConvertMeshToNative
In general I think from my non-codebase owner eye, there are many areas where more exceptions may be raised and are not handled. This PR addresses the generic cases where they were.