SetRelationship (in CommitObjectBuilder) throws "an item with the same key has already been added" exception about speckle-sharp HOT 6 CLOSED

@JR-Morgan
Sure, just created a PR for you dev branch.

from speckle-sharp.

🤔 I think you're right, something's a bit fishy with this if block.

I wonder if we could remove the ContainsKey check all together, and simply use Converted[appId] = conversionResult;
@connorivy what do you think?

from speckle-sharp.

Thanks for looking into this.

Not quite sure if this helps, but after changing the code block to this:

if (dict.ContainsKey)
  dict[key] = value;
else 
  dict.Add(key, value)

my Revit project which I use for testing is converted much faster as before (because no exceptions are thrown, which added quite a lot of overhead to execution time).
Also, both parts of the if/else blocks are hit during debugging, so the logic does either add or change values.
Also, when the method is hit in my tests for the first, the dictionary itself is empty, so assignments won't work until some keys are added.

But I don't understand what the method is supposed to do at all, or what it's purpose is. Just thought I add this here to support you guys somehow.

from speckle-sharp.

If you change your code snippet to just:

dict[key] = value;

with no if check atall, does this still work for you?

from speckle-sharp.

@JR-Morgan
Sorry, my fault.
I was so used to writing the "if not contains key, then add, else assing" pattern in my own code, that I oversaw that the dictionary already has this build in when you assign a key directly as you proposed.
You are right, this will also work, sorry for the confusion.

from speckle-sharp.

No worries, I think we made the same mistake here.

Would you be comfortable submitting a PR to this repo with the same change?

from speckle-sharp.