search

redruin1 / factorio-draftsman

A complete, well-tested, and up-to-date module to manipulate Factorio blueprint strings. Compatible with mods.
MIT License
92 stars 17 forks source link

Add option to extend entity list along with a note about performance #116

Closed RosieBaish closed 7 months ago

RosieBaish commented 8 months ago

Add an extend option to EntityList.

I'm not certain of what @utils.reissue_warnings is doing, but other functions have it so I included it. Hope this is the correct option.

redruin1 commented 7 months ago

Thanks for the PR, I'll take a look at this over the weekend.

utils.reissue_warnings propagates warnings issued in other functions outwards so that the warning points to the outermost calling function. This is done so that methods that issue warnings can be used internally by the module, but don't point to lines internal to the module and instead point to users code. Using it here is correct, as it will mean that warnings will point to where extend() is used (normally in a user's script) instead of in the body of extend() (which is likely an implementation detail), which I believe is more user-friendly and is consistent with the rest of the module.

Basically, the rule of thumb is that if you're writing a method that calls other functions that can issue warnings (such as append() in this case), but want to keep the method you're writing a "black box" and not have warnings point users to the body of said black box, then use reissue_warnings.

redruin1 commented 7 months ago

After taking a look, technically extend is already inherited by MutableSequence, but giving it kwargs for copy and merge (as well as a friendly description) certainly doesn't hurt.

I'll merge this now, thanks again for the contribution.

EDIT: Forgot to mention about the += case; I'll probably add a custom __iadd__ function that will probably just call extend, so theoretically you shouldn't take a performance hit from using the syntactic sugar.