mozilla / elasticutils

[deprecated] A friendly chainable ElasticSearch interface for python
http://elasticutils.rtfd.org
BSD 3-Clause "New" or "Revised" License
243 stars 76 forks source link

Support overriding DEFAULT_INDEXES #171

Closed koterpillar closed 11 years ago

koterpillar commented 11 years ago

Setting elasticutils.DEFAULT_INDEXES does nothing due to the fact that the default value is recorded as None on the compile stage. Besides, putting a (potentially) mutable value as a default is bad practice.

willkg commented 11 years ago

You're right in that argument defaults shouldn't be mutable, but that's not the case here--it's None. DEFAULT_INDEXES isn't intended to be overridden--you should instead subclass S and provide your own get_indexes() implementation.

Generally, changing module-level properties is a bad idea since they get set at module load time.

Unless you have a really compelling reason to do what you're trying to do instead of subclassing S and overriding get_indexes(), I'm going to close this PR because this isn't really what we want to be doing.

koterpillar commented 11 years ago

In many project, module-level properties are actually configuration, I thought that was the case here (especially after successfully overriding DEFAULT_URL). Shouldn't this constant be removed altogether then, not to give a misleading impression?

willkg commented 11 years ago

I don't think I know of any projects that have module-level configuration like this where you change things after the module has been loaded. Usually you have to create your own configuration file like settings.py in Django. That's different.

That constant isn't documented anywhere, so I don't consider it part of the API. I don't think I want to add this to the API--these are really intended to be internal only. Ergo, I'll add a comment that says, "Don't change these, please." I think that covers this and the other constants in that module which are all in a similar situation.

Thank you!

koterpillar commented 11 years ago

Thanks for the explanation!