Switch to pathlib and implement annotations #80

etienne-monier · 2024-01-31T16:10:05Z

This PR use pathlib instead of os + os.path.

Some type hint were added to functions.

Newline argument was added to write_text in Python 3.10

PatrickMassot

J'ai lu en diagonale et pinaillé sur quelques trucs. Par ailleurs il faudrait sans doute ajouter mypy en intégration continue maintenant qu'il y a des annotations de type.

PatrickMassot · 2024-01-31T16:42:18Z

kalamine/cli_xkb.py

+
+if __name__ == "__main__":
+    cli()
+    cli()


Cette ligne dupliquée est une typo, n'est-ce pas ?

PatrickMassot · 2024-01-31T16:48:18Z

kalamine/server.py


+import click


Même commentaire que plus haut.

PatrickMassot · 2024-01-31T16:50:01Z

kalamine/xkb_manager.py


+import click


Même commentaire que plus haut.

PatrickMassot · 2024-01-31T16:51:04Z

kalamine/layout.py


+import click


Je trouve un peu étrange d'importer click dans un fichier qui n'est pas le client en ligne de commande. Je vois que c'est utilisé pour avoir click.echo. Pourquoi ne pas utiliser le module logging de la bibliothèque standard Python plutôt ?

PatrickMassot · 2024-01-31T16:54:22Z

kalamine/xkb_manager.py

-        elif line.startswith("// LAFAYETTE::"):  # obsolete marker
-            return "lafayette"
-    return None
+def is_new_symbol_mark(line: str) -> Union[str, None]:


Pourquoi utiliser Union[str, None] plutôt que Optional[str] ? Par ailleurs je trouve le nom de cette fonction un peu contre-intuitif. Vu le nom j'attendrais bool comme type de retour.

Je confirme, le nom est totalement idiot. Je ne me souviens même plus pourquoi ça s’appelle comme ça.

PatrickMassot · 2024-01-31T16:55:06Z

kalamine/layout.py

@@ -351,8 +348,10 @@ def _fill_template(self, template, rows, layer_number):

        return template

-    def _get_geometry(self, layers=[Layer.BASE]):
+    def _get_geometry(self, layers: Union[List[Layer], None] = None) -> str:


Pourquoi Union[List[Layer], None] plutôt que Optional[List[Layer]] ?

PatrickMassot · 2024-01-31T16:56:53Z

kalamine/layout.py

        """`geometry` view of the requested layers."""
+        if layers is None:
+            layers = [Layer.BASE]


Est-ce vraiment ce qu'on veut ou bien layers = layers or [Layer.BASE] serait mieux ? La différence principale vient du cas où layers est [].

PatrickMassot · 2024-01-31T16:59:33Z

kalamine/cli.py

    """Convert TOML/YAML descriptions into OS-specific keyboard drivers."""

    for input_file in layout_descriptors:
        layout = KeyboardLayout(input_file)

        # default: build all in the `dist` subdirectory
        if out == "all":
-            make_all(layout, "dist")
+            make_all(layout, Path(__file__).parent.parent / "dist")


Ce changement a l'air d'aller au-delà de l'utilisation de pathlib et des annotations. Je n'ai pas vérifié ce qu'il fait.

Merci du heads-up. En fait on ne veut pas de cette ligne.

C’est bourrin mais par défaut, kalamine génère tous les pilotes dans un sous-répertoire dist du répertoire courant. Ça mériterait certainement une option (e.g. --output-dir), mais on ne veut pas générer les fichiers dans le dossier de kalamine.

fabi1cazenave

Merci pour le boulot, j’apprends plein de choses ! Le seul point qui m’apparaisse bloquant c’est l’histoire du répertoire de sortie dist.

Je n’ai pas d’avis pour click (même si le commentaire de Patrick me parle). Je ne savais même pas qu’il pouvait faire ça.

Si on peut utiliser Optional j’avoue que ça me parle plus que Union[*, None], mais c’est vous les spécialistes.

fabi1cazenave · 2024-01-31T19:52:24Z

kalamine/cli.py

    """Convert TOML/YAML descriptions into OS-specific keyboard drivers."""

    for input_file in layout_descriptors:
        layout = KeyboardLayout(input_file)

        # default: build all in the `dist` subdirectory
        if out == "all":
-            make_all(layout, "dist")
+            make_all(layout, Path(__file__).parent.parent / "dist")


Merci du heads-up. En fait on ne veut pas de cette ligne.

C’est bourrin mais par défaut, kalamine génère tous les pilotes dans un sous-répertoire dist du répertoire courant. Ça mériterait certainement une option (e.g. --output-dir), mais on ne veut pas générer les fichiers dans le dossier de kalamine.

fabi1cazenave · 2024-01-31T19:53:36Z

kalamine/cli.py

+""".format(
+            layer_name or layout_layer,
+            "\n".join(getattr(get_layout(layout_name), layout_layer)),
+        )


(Chipotage…)

Là j’avoue que j’avais fait à coups de += pare que je n’avais rien trouvé qui fonctionne sans péter l’indentation — ce qui, sur mon environnement de boulot, pète le repliement de code. Il y a peut-être une ruse de dedent qu’on pourrait utiliser ici ?

fabi1cazenave · 2024-01-31T19:54:16Z

kalamine/cli_xkb.py

-        finally:
-            temp_file.close()
+        temp_file.write(layout.xkb)
+        os.system(f"xkbcomp -w0 {temp_file.name} $DISPLAY")


J’en conclus donc qu’il n’y a pas besoin de fermer un fichier temporaire ? Le système s’en débarrasse de toute façon ?

fabi1cazenave · 2024-01-31T19:57:04Z

kalamine/layout.py

+            return any(
+                self.layers[layer_index][layer_id] == char
+                for layer_id in self.layers[layer_index]
+            )


Joli. :-)
Je crois que layer_index est de type Layer.

fabi1cazenave · 2024-01-31T20:01:55Z

kalamine/layout.py

-            self.dead_keys[dk_id]["base"] = used_base
-            self.dead_keys[dk_id]["alt"] = used_alt
+            dk_dict["base"] = used_base
+            dk_dict["alt"] = used_alt


Bêtement, toute cette boucle for pète dans le refacto et va donc disparaître. Mais pour ma compréhension : pourquoi le nom unknown_element ? Ici l’idée était de parcourir un tableau de caractères (base), donc on sait assez précisément ce que l’élément contient, non ?

(Je note le coup du enumerate, j’adore.)

fabi1cazenave · 2024-01-31T20:07:11Z

kalamine/template.py

+        deadkeys[index]["\u0020"] = dk["alt_space"]
+        deadkeys[index]["\u00a0"] = dk["alt_space"]
+        deadkeys[index]["\u202f"] = dk["alt_space"]
+        if index == ODK_ID:


Je profite d’avoir des gens expérimentés pour essayer de progresser…

Ici j’ai utilisé id parce que la clé du dictionnaire est un identifiant de touche morte (e.g. **, *^, *', etc.) et non un indice. Mais je fais peut-être une erreur de sémantique ?

(Tout ce code étant au cœur du refacto, il va sauter de toute façon.)

fabi1cazenave · 2024-01-31T20:08:36Z

kalamine/xkb_manager.py

-        elif line.startswith("// LAFAYETTE::"):  # obsolete marker
-            return "lafayette"
-    return None
+def is_new_symbol_mark(line: str) -> Union[str, None]:


Je confirme, le nom est totalement idiot. Je ne me souviens même plus pourquoi ça s’appelle comme ça.

let’s merge this first and think about it later

fabi1cazenave · 2024-02-01T15:21:56Z

kalamine/layout.py

@@ -213,7 +209,7 @@ def __init__(self, filepath):
    def _parse_dead_keys(self, spc):
        """Build a deadkey dict."""

-        def layout_has_char(char):
+        def (char: str) -> bool:


j’ai bêtement fait sauter le nom de la fonction en rebasant depuis le navigateur… boulet…

fixing a typo I did while rebasing in the web interface

fix a quick typo

str/Path mismatch

fix a rebasing mistake

refactor: Switch to pathlib and implement annotations

1fc29e6

etienne-monier force-pushed the refactor/pathlib-and-annotations branch from d80f8ee to 1fc29e6 Compare January 31, 2024 16:12

fix: Fix file write in CLI

79bf0b0

Newline argument was added to write_text in Python 3.10

PatrickMassot reviewed Jan 31, 2024

View reviewed changes

fabi1cazenave previously requested changes Jan 31, 2024

View reviewed changes

Merge branch 'master' into refactor/pathlib-and-annotations

faacd6c

fabi1cazenave reviewed Feb 1, 2024

View reviewed changes

fabi1cazenave added 4 commits February 1, 2024 16:26

Update layout.py

35a3772

fixing a typo I did while rebasing in the web interface

Update cli_xkb.py

6d7cb47

fix a quick typo

Update cli.py

affc8ae

str/Path mismatch

Update test_serializer_klc.py

805e902

fix a rebasing mistake

fabi1cazenave merged commit 3da728f into OneDeadKey:master Feb 1, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to pathlib and implement annotations #80

Switch to pathlib and implement annotations #80

etienne-monier commented Jan 31, 2024

PatrickMassot left a comment

PatrickMassot Jan 31, 2024

PatrickMassot Jan 31, 2024

PatrickMassot Jan 31, 2024

PatrickMassot Jan 31, 2024

PatrickMassot Jan 31, 2024

fabi1cazenave Jan 31, 2024

PatrickMassot Jan 31, 2024

PatrickMassot Jan 31, 2024

PatrickMassot Jan 31, 2024

fabi1cazenave Jan 31, 2024 •

edited

Loading

fabi1cazenave left a comment

fabi1cazenave Jan 31, 2024 •

edited

Loading

fabi1cazenave Jan 31, 2024

fabi1cazenave Jan 31, 2024

fabi1cazenave Jan 31, 2024

fabi1cazenave Jan 31, 2024

fabi1cazenave Jan 31, 2024

fabi1cazenave Jan 31, 2024

fabi1cazenave Feb 1, 2024


		import click


		import click


		import click

Switch to pathlib and implement annotations #80

Switch to pathlib and implement annotations #80

Conversation

etienne-monier commented Jan 31, 2024

PatrickMassot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabi1cazenave Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

fabi1cazenave left a comment

Choose a reason for hiding this comment

fabi1cazenave Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabi1cazenave Jan 31, 2024 •

edited

Loading

fabi1cazenave Jan 31, 2024 •

edited

Loading