Feature new date fields #41
Conversation
…cKeyField`. Fixed all Flake8 errors. Updated README. Added unit tests for fields and lookups that are implemented.
|
The first run of Travis tests failed because |
|
|
||
| def __init__(self, lhs, rhs): | ||
| """Implementing an abstract class.""" | ||
| super(DateLookupBase, self).__init__(lhs, rhs) # pragma: no cover |
There was a problem hiding this comment.
As we do nothing in __init__ except calling super, we don't need this method, you can remove it.
There was a problem hiding this comment.
If I remove it, I get warnings that I haven't implemented all abstract methods for PEP8 but more than willing to remove it.
| def __init__(self, input_formats=None, *args, **kwargs): | ||
| """Init that pops off the max_length attribute.""" | ||
| kwargs.pop('max_length', None) | ||
| super(DateField, self).__init__(input_formats, *args, **kwargs) |
There was a problem hiding this comment.
This is python 3, you can remove the argument passed to self they would be filled by the compiler atomatically see https://docs.python.org/3/library/functions.html#super
Same comment apply for the DateTimeField class below and in pgcrypto.mixins.
There was a problem hiding this comment.
Consider it done... used to support py2 and py3 stuff at the same time.
| import factory | ||
|
|
||
| from .models import EncryptedModel | ||
|
|
||
|
|
||
| class EncryptedModelFactory(factory.DjangoModelFactory): | ||
| """Factory to generate hashed and encrypted data.""" | ||
| class Meta: | ||
| class Meta: # NOQA |
There was a problem hiding this comment.
noqa is not needed in the tests module as it's ignored by coverage.
This is what coverage report (from travis):
Name Stmts Miss Cover Missing
------------------------------------------------------
pgcrypto/__init__.py 8 0 100%
pgcrypto/aggregates.py 26 2 92% 59-65
pgcrypto/fields.py 38 0 100%
pgcrypto/forms.py 9 0 100%
pgcrypto/lookups.py 64 4 94% 60-63
pgcrypto/mixins.py 47 0 100%
pgcrypto/models.py 0 0 100%
pgcrypto/proxy.py 20 0 100%
------------------------------------------------------
TOTAL 212 6 97%
There was a problem hiding this comment.
Same comment apply for EncryptedModel.Meta in tests.models.
There was a problem hiding this comment.
Consider it done.... There must of been a change of behavior in coverage since that requirement was upgraded yesterday. The #NOQA was needed in the past otherwise it complained about those lines.
| @@ -0,0 +1,93 @@ | |||
| # Created by .ignore support plugin (hsz.mobi) | |||
There was a problem hiding this comment.
Is it possible to remove this .gitignore? I think a few of the references are configuration specific. We might need a .gitignore to ignore files generated by the library but I think that most of the other cases can be fixed with a global gitignore.
See .gitignore from Django and the github doc about the global gitignore.
There was a problem hiding this comment.
Yea, if you'd like it removed -- can do. We've used a rather broad coverage on .gitignore in our group in the past because different system configurations and not everyone notifies when some tool dumps an extra file.
I think some .gitignore file should be included however for now I will delete it. For example, when running unit tests a file for coverage results .coverage is generated. For those contributing to this project, they may not notice that file be added and accidentally commit it.
|
Peter thank you very much for your contribution, it looks very good. Most of my comments are nitpicks I couldn't see anything to change in general. Once these comments are fixed I'll have a second read and if it's all good we can merge it to master :) |
|
@kevinetienne I committed all the changes you suggested in the review. |
|
Nice one! I'll do a release soon. |
|
Release under |
DatePGPSymmetricKeyFieldandDateTimePGPSymmetricKeyFieldincluding support for serializing / deserializing django form fields