Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updating ark_2.py #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

eonm-pro
Copy link

@eonm-pro eonm-pro commented Feb 4, 2021

Super TP, très instructif et clair ! J'espère qu'il y aura une suite dans la même veine ou sur des sujets proche. Il m'a motivé à faire un projet similaire écrit en Rust qui intègre la création de modules natifs pour Nodejs et Python3.

Je n'ai pas beaucoup d'expérience avec python, les modifications que je peux proposer ne sont donc peut-être pas les plus pythonic.

Les modifications de cette PR portent principalement sur la gestion des erreurs et sur le comportement de la fonction valider.

Les erreurs

  • Les erreurs ont été transformées en véritable type. Le fait de créer des types d'erreur permet de pouvoir les gérer avec try except, comme le fait votre script avec les erreurs fournies par Python : ValueError
       try:
         # ....
       except ValueError:
          valide = self.valider()
           controle = ALPHABET[addition % 29]
           if not valide[0]:
               return controle
               print(valide[1], file=sys.stderr)
           return ''

Avoir de véritables erreurs est très utile pour les développeurs qui utiliseront votre script 😉 (ou vous-même si vous utilisez ce script depuis un autre)

  • Dans la même veine les erreurs sont propagées jusqu'à la fonction main qui s'occupe de les afficher. En d'autres termes, les erreurs ne sont plus affichées avec print au sein même des fonctions. En effet, que se passerait-il si un développeur ou vous-même souhaitiez réutiliser ce script dans un autre projet et qu'au lieu d'afficher les erreurs vous souhaitiez les cacher ?
  • Deux contrôles ont été rajoutés sur la longueur des identifiants InputLengthTooLongError et InputLengthTooShortError. Ils vérifient respectivement que la chaîne de caractères testée n'est ni trop courte ni trop longue. La spécification du NCDA semble indiquer que la chaîne de caractères (sans son caractère de contrôle final) doit avoir une longueur inférieure à la taille de « l'alphabet de contrôle » (R) .

For the algorithm to work, the substring in question must be less than R characters. The extended digit set used in the current instance is a sequence of R=29 printable characters defined as follows
https://metacpan.org/pod/distribution/Noid/noid#NOID-CHECK-DIGIT-ALGORITHM

Mais peut-être qu'il s'agit d'une erreur d'interprétation de ma part, ou bien même que cette partie de la spécification n'est pas en vigueur dans les ARK BNF.

Les fonctions

⚠️ La fonction valider de cette PR ne correspond plus du tout à l'implémentation originelle. Dans cette implémentation elle contrôle la validité de l'identifiant.

Deux remarques sur la syntaxe :

unqualified_id = self.valeur[:-1]

permet de récupérer le début de la chaîne de caractères sans le dernier caractère. À l'inverse :

checksum_char = self.valeur[-1]

permet de récupérer le dernier caractère.

Le contrôle des erreurs est délégué entièrement à la fonction calculer_controle

La fonction calculer_controle pour sa part a été simplifiée, elle ne fait plus appel à l'ancienne fonction valider pour déterminer la validité d'un caractère. Elle s'appuie exclusivement sur le retour d'erreur de ALPHABET.index(carac) pour déterminer que l'identifiant contient un caractère invalide. La contrepartie de cette approche, même si elle est plus rapide, est qu'il n'y a plus de rapport détaillé de l'ensemble des caractères invalides puisque la fonction s'arrête à la première erreur trouvée. Je comprendrais que cela puisse être considéré comme une régression par rapport à l'implémentation originelle.

Bien cordialement

@BertrandCaron
Copy link
Owner

Bonjour @eonm-abes ! Je n'espérais plus reprendre ce sujet, mais l'été me donne l'occasion de remettre un peu le nez dans le sujet python...

Merci beaucoup pour les suggestions que je crois comprendre et qui vont vraiment dans le sens de plus de concision et d'efficacité.

Reste quand même un problème de conception que j'avais relevé à l'époque mais que je n'arrivais pas à résoudre de manière satisfaisante. Dans votre proposition, le module tente de faire deux choses distinctes sur deux objets également différents :

  • Générer un caractère de contrôle sur une chaîne de caractères (supposée présenter uniquement des caractères acceptés) ;
  • Contrôler la validité d'une chaîne de caractères ARK BnF (supposée comprendre comme dernier caractère un caractère de contrôle). (À l'origine, je pensais d'ailleurs ajouter cette fonctionnalité dans une étape suivante du TP.)

Je me souviens m'être demandé s'il ne fallait pas deux classes distinctes, afin de ne pas avoir différents présupposés pour chacune des fonctions. D'où ma façon de faire initiale, plus lourde mais qui tentait de coller à une certaine logique de modélisation. Est-ce que ça vous semble poser un problème ou pas ?

@BertrandCaron
Copy link
Owner

A un certain moment, j'avais imaginé utiliser l'héritage : une chaîne ARK BnF est bien une chaîne ARK composée des seuls caractères autorisés dans l'alphabet en base 29. Mais cela n'est guère plus logique : calculer un caractère de contrôle pour une chaîne ARK BnF qui en comprend déjà un n'a pas tant de sens non plus !

@BertrandCaron
Copy link
Owner

En fait, je pense que la fonction "générer un caractère de contrôle" est utile (pour d'assez mauvaises raisons...) aux personnes qui ont besoin de déduire un ARK d'un numéro de notice (cette opération résulte à mon avis d'un vrai problème de politique de la BnF, qui maintient deux identifiants en parallèle, dont un qui est vu comme déductible de l'autre...).

Mais mon idée, que je n'ai jamais menée plus loin, était de créer plutôt un petit gestionnaire d'identifiants, qui aurait des fonctions de validation, de génération d'identifiants avec un caractère de contrôle déjà intégré, d'association de métadonnées (libellé de la ressource, URL d'accès, date d'attribution, etc.).

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