CNX-9775 sending revit elements via dynamo

[oguzhankoral]

We have assumption on this user has installed Revit connector, otherwise we will be missing RevitSharedResources

[AlanRynne]

Just a couple comments 🙌🏼

[AlanRynne]

Why local function? 🙂

[oguzhankoral]

😓 I asked this myself too while writing.. We use creating param data under both condition of revitDocumentAggregateCache is null or not. And this param data parameters come from preprocessor directives as below, then it was more painful to extract this as private global function..

#if REVIT2020
    DisplayUnitType unitTypeId = default;
#else
    ForgeTypeId unitTypeId = null;
#endif

[AlanRynne]

This makes me a bit supicious... we were previously calling GetAllObjects instead of this and I'm not sure the reason why we did this.

Your change seems legit though... just wondering if we're missing something here.

[oguzhankoral]

This is also my most suspicious place, regardless I believe we should test this with real version too, on both Revit and Dynamo to understand we are having side effect on anything. 🤞

[AlanRynne]

could be simpler if we flip the condition? could somewhat reduce nesting and make the logic a bit more linear.

The 2 return statements above this one could also be collapsed into a return categoryInfo ?? SHC.Undefined which I consider neater (super personal opinion here...)

[AlanRynne]

If this turns into more than a 30 second refactor, disregard it.

[oguzhankoral]

I guess you were referencing to this body (from 156-163) right? Confused because your comment targeting to 166-169

if (categoryInfo != null)
      {
        return categoryInfo;
      }
      else
      {
        return SHC.Undefined;
      }

[AlanRynne]

Is this safe? This means that it will always return null if the cache is not set, which is not immediately obvious.

[oguzhankoral]

The logic was around this as below so I thought it is safe to return immediately if even cache is null....

var types = revitDocumentAggregateCache
      .GetOrInitializeWithDefaultFactory<List<ElementType>>()
      .GetOrAddGroupOfTypes(typeInfo);

    if (!types.Any())
    {
      var name = typeof(T).Name;
      if (element["category"] is string category && !string.IsNullOrWhiteSpace(category))
      {
        name = category;
      }

      appObj.Update(
        status: ApplicationObject.State.Failed,
        logItem: $"Could not find any loaded family to use for category {name}."
      );

      return default;
    }