Warnings/fix/navisworks #3083

jsdbroughton · 2023-11-30T16:59:08Z

This global PR tackles many warnings to pre-empt the more granular initiative.

It is understood to address:

CA1032: Implement standard exception constructors
CA1051: Do not declare visible instance fields
CA1508: Avoid dead conditional code
CA1724: Type names should not match namespaces
CA1854: Prefer 'IsEmpty' over 'Count' to determine whether the collection contains any items
CA1859: Avoid unnecessary lambda captures
CA1860: Use Enumerable.Any() to determine whether any item exists in a collection
CA1861: Do not use 'Enumerable.Last()' to determine whether any item exists in a collection
IDE0004: Cast is redundant
IDE0010: Add missing cases to switch statements
IDE0033: Use explicitly provided tuple name
IDE0036: Order modifiers
IDE0044: Make field read-only
IDE0047: Remove unnecessary parentheses
IDE0059: Unnecessary assignment of a value
IDE0066: Convert switch statement to expression

3 suppressions have been introduced with Justifications:

CA1812: Avoid uninstantiated internal classes: for the two classes that Navis will instantiate at runtime - this is the plugin pattern
CA1508: Avoid dead conditional code: for one class that adds condition handlers not yet migrated to UI options. Yes, this is a partial implementation floating around.
CA1031: Do not catch general exception types: For a single instance where Avalonia is initiated, I have no clue what exceptions that may throw.

* fix(core): IDE0005 Redundant Using statements * fix(ci): GenerateDocumentationFile is required for IDE0005 to run on CI * fix(ci): Allow warnings on all CI jobs

* fix(gh): IDE0005 for Grasshopper connector * fix(rhino): IDE0005 for Rhino connector * fix(rh/gh): IDE0005 for Rhino/Grasshopper shared converter

* remove connector usings * remove converter usings * remove revit shared resources usings * dxfConverter * remove more usings * remove unused statement * is this all of them? --------- Co-authored-by: Connor Ivy <connor@speckle.systems>

* fix(dyn): IDE005 first run * fix(dyn): Fix dangling warning

* fix(tekla): IDE0005 Redundant using statements * fix(structuralUtils): IDE0005 Redundant Using statements

- Remove unused using statements - Clean up namespace declaration

Update variable names in ConnectorNavisworksBindings.cs to follow naming conventions. Update _doc to s_doc, _cachedCommit to s_cachedCommit, and _cachedState to s_cachedState.

The try-catch block swallows unknown exceptions in the InitAvalonia instance of the CreateControlPane method. Introduced as a justified suppression

Refactor code to use discard variable (`_`) instead of unused variable (`unused`) in ConverterNavisworks.ToSpeckle.cs. This change is to a different convention to eliminate the warning generated by the IDE.

- Changed return type of GetObjectsFromSelection and GetObjectsFromSavedSets methods from IEnumerable<ModelItem> to HashSet<ModelItem> - Changed return type of TranslateFragmentGeometry method from IReadOnlyList<Base> to List<Base> - Changed return type of MoveAndScaleVertices method from IEnumerable<double> to List<double> - Changed return type of ViewpointToBase method from Base to View3D

Refactor code to use `static readonly` instead of `readonly static` for dictionary and string variables in multiple files. This improves code readability and follows best practices.

Refactor code to use `Count` property instead of `Any()` method for checking if a collection is empty. This change was made in multiple files: ConnectorNavisworksBindings.cs, ConnectorNavisworksBindings.Filters.cs, ConnectorNavisworksBindings.Send.cs, SelectionBuilder.cs, ConverterNavisworks.Properties.cs, and ConverterNavisworks.ToSpeckle.cs.

Mostly adding redundant default conditions. - In ConnectorNavisworksBindings.Filters.cs, added a default case to handle savedItem.IsGroup == true. - In Autosave.cs, added a default case with an empty comment to cover all logical scenarios for a boolean 'enable'. - In SelectionBuilder.cs, added a default case in the switch statement to throw an ArgumentOutOfRangeException for an unrecognized filter type. Also added a default case in the switch statement to handle SDK object scenarios.

Refactor code to use a static readonly array for `lowerBounds` and `separator`. This improves code readability and maintainability by centralizing the values used in multiple places.

The variable "element" is now assigned from `pair.Value.Key` instead of `pair.Value.Item2` use of the explicit Tuple value provided.

Refactor ConverterNavisworks.Settings.cs to use `TryGetValue` instead of `ContainsKey` and indexer access for retrieving values from the Settings dictionary. This improves code readability and reduces unnecessary dictionary lookups.

Refactor switch statement to use switch expression in ConverterNavisworks.Other.cs. This commit replaces the switch statement with a switch expression to simplify the code and improve readability. The `renderColor` variable is now assigned based on the value of `materialSettings.Mode`, with corresponding cases for "`active`", "`permanent`", and "`original`". A default case has also been added to handle any other values.

Refactor code to remove unnecessary parentheses.

Adds a suppression as the switch expression includes the method solutions for future development feature already scoped and partially implemented.

Add [SuppressMessage] attribute to internal classes in Ribbon.xaml.cs and SpeckleNavisworksCommand.cs to avoid CA1812 warning. These classes are the Plugin and the Ribbon which will be instantiated by Navisworks at runtime.

- Remove cast to `Action` consequently - Remove unnecessary line breaks and indentation in the Invoke method call.

variable `_settingsHandler` was not marked as `readonly`.

Change the access modifier of `ProgressBar` field in `SelectionBuilder.cs` to `internal`. This fixes the Code Analysis warning CA1051: Do not declare visible instance fields

Refactor `Utilities` class to `SpeckleNavisworksUtilities` to avoid clash with `Autodesk.Navisworks.Gui.Utilities`. Update references in ConnectorNavisworksBindings.cs, ConnectorNavisworksBindings.Send.cs, and SpeckleStreamManager.cs to use the new class name.

jsdbroughton · 2023-11-30T17:12:00Z

@AlanRynne i targeted this at dev as requested, but having branched from IDE0005 it includes some commit noise from the merged fixes contained prior. It passes CI, but let me know if you need me to finesse this a bit. Your GitFu may be needed.

BovineOx

Happy for this to go as-is but worth reviewing my comments as it throws up a few Qs on some of the existing code.

BovineOx · 2023-12-01T08:58:58Z

ConnectorNavisworks/ConnectorNavisworks/Bindings/ConnectorNavisworksBindings.Filters.cs

@@ -142,6 +142,8 @@ private static TreeNode GetViews(SavedItem savedItem)
        return null;
      case false:
        return treeNode;
+      default:


Kind of missing the context here, might look at the file more completely but I was wondering about this default: break; looking like the default is to continue and wondering if the switch statement made sense still?

BovineOx · 2023-12-01T09:00:56Z

ConnectorNavisworks/ConnectorNavisworks/Bindings/ConnectorNavisworksBindings.Filters.cs

    {
      return node;
    }

    var elements = node.Elements.Select(RemoveNullNodes).Where(childNode => childNode != null).ToList();

-    if (!elements.Any())
+    if (elements.Count == 0)


Given we've converted the LINQ to a list, Count should be fine but Any() will be faster in other places. What warning was this fixing?

BovineOx · 2023-12-01T09:04:45Z

ConnectorNavisworks/ConnectorNavisworks/Entry/Commands.cs

@@ -2,27 +2,27 @@ namespace Speckle.ConnectorNavisworks.Entry;

 public abstract class LaunchSpeckleConnector


Outside of review scope really, but these abstract classes look a bit funky.

BovineOx · 2023-12-01T09:06:09Z

ConnectorNavisworks/ConnectorNavisworks/Entry/SpeckleNavisworksCommand.cs

    "Speckle",
    DisplayName = "Speckle",
    Options = PluginOptions.None,
    ToolTip = "Speckle Connector for Navisworks",
    ExtendedToolTip = "Speckle Connector for Navisworks"
  )
 ]
-// ReSharper disable once ClassNeverInstantiated.Global - This is because of the mechanism Navisworks uses to load plugins.
+[SuppressMessage(


Nice, very clear 👍🏻

BovineOx · 2023-12-01T09:07:17Z

ConnectorNavisworks/ConnectorNavisworks/NavisworksOptions/Autosave.cs

@@ -36,6 +36,10 @@ private void UpdateAutoSaveSetting(bool enable)
      case true when _autosaveSetting:
        rootOptions.SetBoolean("general.autosave.enable", true);
        break;
+      default:


Again this makes me wonder if the switch use is a bit janky here...

BovineOx · 2023-12-01T09:09:08Z

ConnectorNavisworks/ConnectorNavisworks/Other/Elements.cs

@@ -37,6 +37,9 @@ private Element(string pseudoId)

  public string PseudoId { get; private set; }

+  private static readonly int[] s_lowerBounds = new[] { 1 };
+  private static readonly string[] s_separator = new[] { "." };


I suppose arrays cannot be changed but we can change the single elements within the arrays. I don't know what they are but curious about them. Are they app domain specific? Should anything be coming from CurrentCulture?

BovineOx · 2023-12-01T09:12:24Z

ConnectorNavisworks/ConnectorNavisworks/Other/SelectionBuilder.cs

        _uniqueModelItems.AddRange(GetObjectsFromSavedViewpoint());
        break;
+      default:


Being picky I'd prefer a line space between the break of the previous case and the default case and generally between break and cases but maybe that's coming soon.

BovineOx · 2023-12-01T09:15:57Z

ConnectorNavisworks/ConnectorNavisworks/Other/SelectionBuilder.cs

@@ -267,6 +265,10 @@ private TreeNode GetViews(SavedItem savedItem)
          treeNode.Elements.Add(GetViews(childItem));
        }
        break;
+      default:


So I think with one or two of these you're saying it has to be handled in a previous switch statement. I would argue if that is the case then the default could throw, because this throw helps validate our belief that it can never be X or Y. More likely, at least when I say this, I am saying it should never get to this default, throwing ensures that this continues to be true and if it ceases to be true, we can spot it.

AlanRynne and others added 27 commits November 28, 2023 16:38

ci: Show all warnings on CI builds

c10a7f5

fix(all): Do not allow IDE0005 on CI

061bd0c

fix(sdk): IDE0005 Redundant Using statements in SDK projects (#3071)

3bb427b

* fix(core): IDE0005 Redundant Using statements * fix(ci): GenerateDocumentationFile is required for IDE0005 to run on CI * fix(ci): Allow warnings on all CI jobs

fix(rhino/grasshopper): Fix IDE0005 Redundant Using Statements (#3072)

aa7937d

* fix(gh): IDE0005 for Grasshopper connector * fix(rhino): IDE0005 for Rhino connector * fix(rh/gh): IDE0005 for Rhino/Grasshopper shared converter

Revit: IDE0005 fix (#3073)

d43f96f

* remove connector usings * remove converter usings * remove revit shared resources usings * dxfConverter * remove more usings * remove unused statement * is this all of them? --------- Co-authored-by: Connor Ivy <connor@speckle.systems>

fix(dyn): IDE0005 Redundant Using Statements (#3074)

9ea20bb

* fix(dyn): IDE005 first run * fix(dyn): Fix dangling warning

fix(CNX-8300): IDE0005 Redundant using statements (#3076)

1e823d4

* fix(tekla): IDE0005 Redundant using statements * fix(structuralUtils): IDE0005 Redundant Using statements

Refactor NavisworksOptions Manager.cs

2aa3ab7

- Remove unused using statements - Clean up namespace declaration

Fix IDE1006

e62472d

Update variable names in ConnectorNavisworksBindings.cs to follow naming conventions. Update _doc to s_doc, _cachedCommit to s_cachedCommit, and _cachedState to s_cachedState.

Fix CA1032

7e985f2

The try-catch block swallows unknown exceptions in the InitAvalonia instance of the CreateControlPane method. Introduced as a justified suppression

FIX IDE0059

783cc7a

Refactor code to use discard variable (`_`) instead of unused variable (`unused`) in ConverterNavisworks.ToSpeckle.cs. This change is to a different convention to eliminate the warning generated by the IDE.

Fix IDE0036

0eceb12

Refactor code to use `static readonly` instead of `readonly static` for dictionary and string variables in multiple files. This improves code readability and follows best practices.

Fix CA1861

ac052d2

Refactor code to use a static readonly array for `lowerBounds` and `separator`. This improves code readability and maintainability by centralizing the values used in multiple places.

FIX IDE0033

d16d641

The variable "element" is now assigned from `pair.Value.Key` instead of `pair.Value.Item2` use of the explicit Tuple value provided.

FIX CA1854

aa7ef81

Refactor ConverterNavisworks.Settings.cs to use `TryGetValue` instead of `ContainsKey` and indexer access for retrieving values from the Settings dictionary. This improves code readability and reduces unnecessary dictionary lookups.

FIX IDE0047

0b569e6

Refactor code to remove unnecessary parentheses.

FIX CA1508

b077e42

Adds a suppression as the switch expression includes the method solutions for future development feature already scoped and partially implemented.

FIX IDE0047

894dca6

Add [SuppressMessage] attribute to internal classes in Ribbon.xaml.cs and SpeckleNavisworksCommand.cs to avoid CA1812 warning. These classes are the Plugin and the Ribbon which will be instantiated by Navisworks at runtime.

FIX IDE0004 : Remove unnecessary Cast

48d8e13

- Remove cast to `Action` consequently - Remove unnecessary line breaks and indentation in the Invoke method call.

FIX IDE0044

8735f28

variable `_settingsHandler` was not marked as `readonly`.

FIX CA1051

81da312

Change the access modifier of `ProgressBar` field in `SelectionBuilder.cs` to `internal`. This fixes the Code Analysis warning CA1051: Do not declare visible instance fields

dang missed one

939d264

jsdbroughton requested review from teocomi and AlanRynne as code owners November 30, 2023 16:59

jsdbroughton removed the request for review from teocomi November 30, 2023 16:59

jsdbroughton requested a review from BovineOx November 30, 2023 16:59

Merge branch 'dev' into warnings/fix/navisworks

78e0754

BovineOx approved these changes Dec 1, 2023

View reviewed changes

AlanRynne merged commit 6c9e573 into dev Dec 1, 2023
30 checks passed

AlanRynne deleted the warnings/fix/navisworks branch December 1, 2023 12:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warnings/fix/navisworks #3083

Warnings/fix/navisworks #3083

jsdbroughton commented Nov 30, 2023

jsdbroughton commented Nov 30, 2023 •

edited

BovineOx left a comment

BovineOx Dec 1, 2023

BovineOx Dec 1, 2023

BovineOx Dec 1, 2023

BovineOx Dec 1, 2023

BovineOx Dec 1, 2023

BovineOx Dec 1, 2023

BovineOx Dec 1, 2023

BovineOx Dec 1, 2023

		@@ -2,27 +2,27 @@ namespace Speckle.ConnectorNavisworks.Entry;

		public abstract class LaunchSpeckleConnector

Warnings/fix/navisworks #3083

Warnings/fix/navisworks #3083

Conversation

jsdbroughton commented Nov 30, 2023

jsdbroughton commented Nov 30, 2023 • edited

BovineOx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdbroughton commented Nov 30, 2023 •

edited