wmayer wrote: ↑
Thu Nov 21, 2019 11:18 pm
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.