Skip to content

Add the module for Ecuadorian Identification Card (CI - Cedula de identi...#12

Closed
jonathanf wants to merge 3 commits into
arthurdejong:masterfrom
jonathanf:master
Closed

Add the module for Ecuadorian Identification Card (CI - Cedula de identi...#12
jonathanf wants to merge 3 commits into
arthurdejong:masterfrom
jonathanf:master

Conversation

@jonathanf

Copy link
Copy Markdown
Contributor

...dad) and Fiscal Numbers (RUC - Registro Unico de Contribuyentes)

Dear Arthur, check out my contribution for the proyect.

Best regards,
Jonathan

…ntidad) and Fiscal Numbers (RUC - Registro Unico de Contribuyentes)
@arthurdejong

Copy link
Copy Markdown
Owner

Thanks. A few comments:

  • the tests current fail (the examples in the docstring do not match the expected output)
    (you should be able to run the tests with nosetests)
  • incorrect formatting of numbers does not raise the correct exceptions:
    ci.validate('123A567890') raises ValueError instead of InvalidFormat
  • in stdnum.ec.ci it is probably clearer to factor out a separate function and reduce it to:
  def calc_check_digit(number):
      """Calculate the check digit. Any check digits present in the number
      are ignored"""
      total = sum((int(number[i]) * (2 - x % 2)) % 9 for i in range(9))
      return str((10 - total) % 10)

(the ruc module can probably also be simplified a bit to use sum() like above instead of for loops)

  • in stdnum.ec.ruc the module docstring says "third digit is a letter" but this isn't reflected in the code

If you run nosetests like above you also get a coverage report that shows lines that are not covered by tests.

Can you make the corrections above? Thanks for your contribution!

@jonathanf

Copy link
Copy Markdown
Contributor Author

Of course, the next week you will have this.

Best regards,
Jonathan

Enviado desde Samsung Mobile

-------- Mensaje original --------
De: Arthur de Jong notifications@github.com
Fecha:09/08/2014 8:18 AM (GMT-05:00)
A: arthurdejong/python-stdnum python-stdnum@noreply.github.com
CC: Jonathan Finlay jfinlay@riseup.net
Asunto: Re: [python-stdnum] Add the module for Ecuadorian Identification Card (CI - Cedula de identi... (#12)
Thanks. A few comments:

the tests current fail (the examples in the docstring do not match the expected output) (you should be able to run the tests with nosetests)
incorrect formatting of numbers does not raise the correct exceptions: ci.validate('123A567890') raises ValueError instead of InvalidFormat
in stdnum.ec.ci it is probably clearer to factor out a separate function and reduce it to:
def calc_check_digit(number):
"""Calculate the check digit. Any check digits present in the number
are ignored"""
total = sum((int(number[i]) * (2 - x % 2)) % 9 for i in range(9))
return str((10 - total) % 10)
(the ruc module can probably also be simplified a bit to use sum() like above instead of for loops)

in stdnum.ec.ruc the module docstring says "third digit is a letter" but this isn't reflected in the code
If you run nosetests like above you also get a coverage report that shows lines that are not covered by tests.

Can you make the corrections above? Thanks for your contribution!


Reply to this email directly or view it on GitHub.

@jonathanf

Copy link
Copy Markdown
Contributor Author

Hey, i hope there is done. Tell me if something is not right!

arthurdejong added a commit that referenced this pull request Oct 17, 2014
Add modules for Ecuadorian Identification Card (CI - Cédula de
identidad) and Fiscal Numbers (RUC - Registro Único de Contribuyentes)

See: #12
@arthurdejong

Copy link
Copy Markdown
Owner

Thanks for the update. I have merged your changes with some modifications.

I can't read Spanish so please review the changes and point out any errors in the code. I've had a look at https://github.com/diaspar/validacion-cedula-ruc-ecuador and a few other documents I found online and understand that a natural RUC (third digit 0..5) is a CI followed by an establishment number. This means that the validation of a natural RUC can be delegated to the CI module.

I've also added tests for more numbers that I found online to ensure that various possible errors in check digit calculations should be covered now (this brought to light a issue in your calculation for natural the RUC).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants