-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add stream parsing in Schema class #25
Conversation
pyproject.toml
Outdated
@@ -33,3 +33,4 @@ exclude = [".git/", ".pytest_cache/", ".venv"] | |||
|
|||
[tool.pytest.ini_options] | |||
python_files = ["tests/*"] | |||
log_cli = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nécessaire pour les tests temporaires pour afficher des logs dans le résultat des tests exécutés avec pytest. Sera enlevé en même temps que les tests.
magicparse/schema.py
Outdated
|
||
return items, errors | ||
|
||
def stream_parse(self, data: Union[bytes, BytesIO]) -> Iterable[Tuple[dict, dict]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pour info, il n'est plus nécessaire d’hériter des types de typing
pour les collections, et il existe une syntaxe dédiée à l'union). le type dict n'est pas non plus complet, mais ça devient un poil complexe au niveau refacto, on peut utiliser Any pour la valeur, la clef par contre devrait tjrs etre des str
, non ?
On pourrait réécrire le type en :
def stream_parse(self, data: bytes | BytesIO) -> Iterable[tuple[dict[str, Any], dict[str, Any]]]:
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je suis d'accord avec toi Mathias !
Mais pour faire ça il faut d'abord bump la version de Python du projet dans le Dockerfile
(passer de 3.9 à 3.11) et faire pareil dans pyproject.toml
.
C'est pas un gros changement et de toute façon le seul service qui consomme magicparse est external-is qui est déjà en Python 3.11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On peut faire ça dans une autre PR en parallèle aussi, c'est pas directement lié à celle ci et ce n'est pas bloquant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quelques remarques mais sinon c'est top !
Faudrait que tu vois avec un gars de ton équipe pour avoir poetry fonctionnel sur ta machine + les hooks de pre commit! |
|
||
class TestStreaming(TestCase): | ||
|
||
def test_stream_parse_errors_do_not_halt_parsing(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
J'ai l'impression que tu tests beaucoup de chose en un :)
Quel est l'intention derrière ce test spécifiquement ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oui, J'ai repris un test existant sur parse qui me semblait testé un cas passant et un cas échouant.
Pour le reste je me suis dit que dupliquer tous les tests n'avait pas de valeur ajoutée sachant que parse appelle stream_parse et me semblait déjà bien couverte.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pas grand chose à dire de plus que les autres ! :)
Une doc que je pourrais suivre pour installer ça ? |
35815c3
to
3fc9254
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Faut juste un passage du formateur black + flake8 et ça devrait passer la CI :)
…nsumption of memory
fb32e77
to
e11a82d
Compare
Pour créer une nouvelle version il te faut:
|
Le but de cette PR est d'ajouter un moyen de lire séquentiellement les lignes parsées d'un fichier au lieu de tout lire d'un coup pour réduire l'empreinte mémoire d'un import de fichier.
Des tests temporaires ont été mis pour vérifier:
J'ai laissé ces tests pour la PR mais je vais les enlever avant le merge: