search

psolin / cleanco

Company Name Processor written in Python
MIT License
318 stars 94 forks source link

Move prepare_terms() inside basename() #71

Closed taraskuzyk closed 2 years ago

taraskuzyk commented 3 years ago

As discussed in issue #70

Made sure that the public API won't be broken from doing this by giving terms a default value of None

petri commented 2 years ago

What is the use case for this? Calling prepare_terms per each time you call basename is very inefficient. Unless we also cache prepare_terms. Another possibility would be to provide an alternative additional basename partial implementation that would already have terms passed to it.

For what it's worth, we probably should also rename prepare_terms into prepare_default_terms since that's what it really is for - users could substitute their own version.

petri commented 2 years ago

I hope you don't mind me creating an alternative PR that basically does what you suggested here, but a bit differently (it turns basename into a partial); see #75.

It turns out that this allowed us to also simplify the tests a bit, so thank you for proposing this!

petri commented 2 years ago

75 was merged that hopefully provides the same benefit as this PR would have.