Add field for greek Social Security Number (AMKA) #337
Conversation
Codecov Report
@@ Coverage Diff @@
## master #337 +/- ##
==========================================
- Coverage 95.89% 95.82% -0.08%
==========================================
Files 153 153
Lines 3851 3878 +27
Branches 512 517 +5
==========================================
+ Hits 3693 3716 +23
- Misses 96 98 +2
- Partials 62 64 +2
Continue to review full report at Codecov.
|
increase coverage
|
Finally, this PR should be ready for merge! As a general notice, submitting code to this repository is way to difficult, it took me like 50 times more time to fix the various satellite things (submission changes, isort imports, prospector warnings, codecov etc) than to write the actual code for the new FormField! I recommend opening an issue for discussing this problem a bit... |
There was a problem hiding this comment.
Thanks for your contribution and sorry for the delay in getting to this. There are a few changes that are needed before this PR can be merged. I'll try to respond quicker when you make an updated version.
Sorry to hear that you had trouble with the code checks. I setup these checks so that the localflavor maintainers don't have to manually look for the code consistency that we're aiming for and so that we don't need to repeat the same feedback for every PR. I understand that it adds a little more work to submitting a PR but I feel it's worth the extra effort to keep the code more consistent across a large number of contributors and to help keep the code quality high. Thanks for understanding.
I'm looking forward to reviewing your updated version. Hopefully we can get this feature into the 2.1 release.
localflavor/gr/forms.py
Outdated
| @@ -60,3 +61,56 @@ def clean(self, value): | |||
| if mod != check: | |||
| raise ValidationError(self.error_messages['invalid']) | |||
| return val | |||
|
|
|||
|
|
|||
| class GRSocialSecurityNumberCodeField(Field): | |||
There was a problem hiding this comment.
It would be better use RegexField instead of Field. You would then set the regex on the call to super.
localflavor/gr/forms.py
Outdated
| except: | ||
| raise ValidationError(self.error_messages['invalid']) | ||
|
|
||
| def check_lunh(self, val): |
There was a problem hiding this comment.
Please use the lunh function that's already in localflavor instead of including your own version.
localflavor/gr/forms.py
Outdated
| def clean(self, value): | ||
| super(GRSocialSecurityNumberCodeField, self).clean(value) | ||
| if value in EMPTY_VALUES: | ||
| return '' |
There was a problem hiding this comment.
I think you won't need this check if you inherit from RegexField since there are checks for empty values in CharField.
There was a problem hiding this comment.
Actually this:
if value in self.empty_values:
return self.empty_value
was needed or else the test test_forms_char_field_empty_value_allows_none would fail.
There was a problem hiding this comment.
Ok cool. Thanks for catching that.
| @@ -14,7827 +14,10656 @@ msgid "" | |||
| msgstr "" | |||
There was a problem hiding this comment.
Please remove the changes to this file. The localflavors take care of updating the translation files (which are managed through Transifex).
There was a problem hiding this comment.
Ok I restored it to the previous version
|
Hello @benkonrath, I've implemented the requested changes, I hope the PR will get merged soon! Concerning my rant about the work needed for the PR, I understand that you want to have some standards for contributed code, however think about how much work is needed for guys that don't do this every day. You probably have setup your environments, tox, isort, prospector etc so that you can write code that passes all the tests automatically however most people don't have that comfort. I consider myself a rather experienced developer and setting up all the things needed to make the contribution and fixing things up was a major PITA; if I knew that it needed so much work just to add a form field (which I already use in my project) then I probably wouldn't bother at all. I had contributed some other fields before some years when each localflavor package was separate (https://github.com/spapas/django-localflavor-gr) the process back then was very easy. In any case, concerning the nature of the project (localflavor needs contributions for many people across the globe; you can't substitute quality for quantity here) I think that it does not make sense to have the same contribution standards as with Django. You need to lower your standards to encourage more contributions. After all, most of these contributions are values lists and form fields. The people that are maintaining this project can pick the individual contributions and make them as good as they want; however you don't want to discourage from contributing by making them jump hoops. I hope you get the idea about my initial rant, really sorry if I ranted again trying to explain my thoughs! Best regards and thanks for your work, |
|
@spapas Thanks again for your contribution. I see your point about making it easier to contribute to localflavor. I think we just have a different value on what's a suitable quality level for this project. Perhaps we could improve the documentation on how to test locally rather than having to wait for Travis to fail. Suggestions or PRs for this would be helpful. I'm not the last word on this. I'm only the most active localflavor maintainer at the moment. If you want to discuss this further, perhaps you could open a new issue about this like you suggested. Just a warning though, I don't know if you'll get much of a response because the other maintainers aren't too active these days. People who watch the repo can also comment here if they have an opinion about this. |
|
It's not easy to draw the balance between code quality and ease of contribution. For me, testing/coverage is a must and we shouldn't lower that quality requirement. It's one of the Django mantras: "untested code is broken code". |
|
Hello @benkonrath thank you very much for merging my PR! Now, for the quality/easy of contribution, I understand your concerns and am all in for code quality. However I agree with @claudep that the bar for contributions is too high for most developers and, due to the nature of the project as explained in my previous comment, it would be better to lower it a bit to encourage more contributions (and merging existing ones). I believe that being able to find some specific form fields for your country is a factor that weights when you are a lone developer in (f.e) Greece and want to decide which framework you'll want to learn! In any case, what was in my mind would be to allow and quickly merge even non perfect PRs that would then be fixed/improved (by adding more tests, complying with PEP8 etc) by the original contributor or the maintainers; you'll add an issue with what is missing and then assign it to the original contributor; if he doesn't fix it after a while then a maintainer will step up and fix it for him. However after Ben's previous comment (that most maintainers are not so active) I'm not sure if that's possible because it will add a lot of effort to the maintainers (to add the corresponding issue and fix it if needed) so probably the contribution workflow should stay as it for now :/ |
[x ] Add an entry to the docs/changelog.rst describing the change.
[Was there ] Add an entry for your name in the docs/authors.rst file if it's not
already there.
[x ] Adjust your imports to a standard form by running this command: