Misleading name: addSAMSitesByPrefix
Codingboy opened this issue · 3 comments
Even though it is documented that the function clears the samsites the name is misleading because it SETS the samsites instead of ADDing them. Please rename it to setSAMSitesByPrefix and consider adding a new function addSAMSitesByPrefix which really adds them.
I once was struggling with it and now I ran into the same issue again just because I forgot about it.
In my case I wanted to add all "red sa" and all "red aaa" and so I called the function twice resulting in only aaa controlled by skynet...
To be fair, that would break all existing scripts, we handle a lot of changes with MOOSE functions and the best approach is to make two completely new functions and handle the legacy function nicely so it doesnt break anything. This kind of short name confusion on methods does do this often, the observation is completley valid, but the solution should be less severe. Just a thought.
In general I agree with you that it has the potential to break scripts.
BUT who really uses this function to change the samsites?
Honestly, marking the function as deprecated and add 2 new functions seems to be the best way.
fair point @Codingboy will add ad setSAM method that behaves like the add one and update the add behaviour. Will add it to the next major version release. In this case I don‘t think it would break existing missions, since its a rather small change in behaviour