Code review of merged Link3 branch

[triplus]

As @realthunder said the ability of overriding all properties would likely be too complex to achieve, i guess "freeze property" could help with that. That is you would first create a link feature and change its "freeze property" to True. After you could change the linked feature parameters and create a new link. You now have two link features, each with different geometry.

The question i guess is, on how much sense and on how hard would it be, to port the "freeze property" from Assembly 3, and to make it a "core link" functionality.

[toralf]

I believe that overriding all properties is not what is requested. Just some parameters of the model. IMHO it would be even acceptable if it is necessary to specify these parameters similar to the OPP-like topo naming approach that realthunder introduced. Meaning the author of the original model specifies which parameters are public (could be changed via link). Then the author could ensure that the model works with a certain range of these parameters. And if with a max/min limit for these public parameters the abuse of the model would be very rare.

[realthunder]

Wow, I am certainly lagging behind with all this interesting discussion. I am working on another big PR now, so sorry for the MIA. I agree the usefulness of a general way of parametric configuration. The question is how to present that capability to user/developer. I'll get back to it once I sort out the PR at hand.

[wmayer]

I am working on another big PR now

Why this again? Can't this be divided into smaller parts? By having a quick look just at the commit messages I have the impression that it's a potpourri of changes here and changes there that are not related at all.
IMO, that's not the way PRs should be used. Instead one PR should contain only the changes for one improvement or new feature but it should not try to handle 100 different issues at the same time.

[realthunder]

I am working on another big PR now

Why this again? Can't this be divided into smaller parts? By having a quick look just at the commit messages I have the impression that it's a potpourri of changes here and changes there that are not related at all.
IMO, that's not the way PRs should be used. Instead one PR should contain only the changes for one improvement or new feature but it should try to handle 100 different issues at the same time.

They are all related for sure. But it is possible to split. I'll write a post to explain the whole thing first. Then I'll see how to split it.

[ezzieyguywuf]

Why this again?

I’m with wmayer here. Personally, I still haven’t made it past the first few commits of “the big merge” (still finalizing a pr to update the logging facilities)

I agree with the sentiment that a given PR should give a distinct incremental improvement/fix/feature.

For example, looking at your first few commits in your GroupMod branch, I wonder - what is going on? Why make extensionIsElementVisible const? Or, better put, what motivated this change? Are there other places in the code base that can also be properly labeled const? If so, why not submit that on its own as a pr? Even if it’s just the one spot.

More alarmingly: why throw an error rather than send a log when calling addDynamicProperty? I’m not too terribly familiar with this method (is it new from “the big merge”?), but this is is a drastic change from the previous behavior. I would 100% expect this to be its own PR, with an explanation behind the motivation and a discussion around if it breaks existing code.