Add store_attributes - Githubissues

hynek / characteristic

Please use attrs instead!

https://attrs.readthedocs.io/

MIT License

84 stars 19 forks source link

Add store_attributes #20

Closed Julian closed 10 years ago

Julian commented 10 years ago

Closes #19

hynek commented 10 years ago

Thanks!

Couple of notes:

It needs a changelog entry.
flake8 is failing.
it’s lacking coverage for lines 169 & 182.
I don’t really understand your line comment. %)

coveralls commented 10 years ago

Coverage decreased (-1.23%) when pulling b93654c6810ba61ab8b7fee4897ef090ab3f7c02 on Julian:master into a8b0abd9f725a7a84371e944ca31a0c4a2d2b15d on hynek:master.

Julian commented 10 years ago

Added changelog and "fixed" the pep8 warnings.

The missing coverage is related to the line comment :). The line comment means "Attribute should be using with_cmp and friends". I can take a look at that, maybe later.

hynek commented 10 years ago

So if you can make it, be my guest but I would be fine with just exercising these two lines. :)

Could you also add a link to this PR in the changelog like in the other instances?

Thanks!

hynek commented 10 years ago

Also an idea: how about not making it configurable such that tools can rely on the existence of it? Is there any pressing reason to make it configurable?

Julian commented 10 years ago

I find it bad manners when things assume they can set public attributes on objects they did not create when they don't provide a way to specify what that attribute should be. Presumably tools that wanted to rely on it existing would use the same sort of configuration as what the decorator itself uses, but if you don't care, I can remove it.