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

Merger gzip #47

Merged
merged 7 commits into from
Jan 15, 2021
Merged

Merger gzip #47

merged 7 commits into from
Jan 15, 2021

Conversation

aollier
Copy link
Contributor

@aollier aollier commented Dec 12, 2020

Pas mal de modifications dans cette pull request dont la plus importante a pour but de resolve #45.

ce sont des commandes shell, pas du code Python ou Bash
ships est maintenant une liste que l'on peut trier en place
@parmentelat
Copy link
Contributor

je ne comprends pas du tout l'intérêt du module factory, est-ce que tu peux élaborer ?

à première vue les deux classes dans file_manager font tout à fait l'affaire, l'introduction d'une factory semble tout à fait superfétatoire non ?

@aollier
Copy link
Contributor Author

aollier commented Dec 13, 2020

Effectivement le module factory n'est pas vraiment indispensable. J'y ai beaucoup réfléchi et j'ai trouvé que c'était plus élégant d'avoir une fabrique qui crée les bonnes instances de FileManager.
Tu l'as compris, j'ai mis en œuvre le patron de conception Fabrique parce que j'ai trouvé que la situation s'y prêtait mais peut-être pas tant que ça.

On pourrait se contenter d'avoir une instance du FileManager qui convient dès le début, et se passer de la Fabrique qui l'instancie. Mais j'ai trouvé cela moins élégant.

@parmentelat
Copy link
Contributor

je reste sur la position qu'il nous faut enlever la couche factory qui n'apporte amha pas grand chose et rend les choses inutilement compliquées

@aollier
Copy link
Contributor Author

aollier commented Dec 21, 2020

Ok, c'est fait !

@aollier
Copy link
Contributor Author

aollier commented Dec 22, 2020

Bon je vais retravailler tout ça parce que j'ai trois commits au lieu d'un seul, et en plus je trouve que le message de validation n'est pas vraiment le bon, donc je vais corriger tout cela d'un coup.

@aollier aollier force-pushed the merger-gzip branch 3 times, most recently from f5c9692 to 8a07cb0 Compare December 24, 2020 17:29
@aollier
Copy link
Contributor Author

aollier commented Dec 26, 2020

J'ai mieux à te proposer : mettre les constantes READ et WRITE sous forme d'énumération tout en gardant une utilisation simple, comme ceci :

import abc
import enum
import gzip


class AbstractFileManager(abc.ABC):

    @classmethod
    def open(cls, filename, mode):
        return cls._open(filename,
            **{"encoding": "UTF-8", "mode": mode, "newline": '\n'})

    @staticmethod
    @abc.abstractmethod
    def _open(filename, **kwargs):
        pass

    @property
    def READ(self):
        return self.Mode.READ.value

    @property
    def WRITE(self):
        return self.Mode.WRITE.value


class GzipFileManager(AbstractFileManager):

    @enum.unique
    class Mode(enum.Enum):
        READ = "rt"
        WRITE = "wt"

    @staticmethod
    def _open(filename, **kwargs):
        return gzip.open(filename=filename, **kwargs)


class TextFileManager(AbstractFileManager):

    @enum.unique
    class Mode(enum.Enum):
        READ = "r"
        WRITE = "w"

    @staticmethod
    def _open(filename, **kwargs):
        return open(file=filename, **kwargs)

Ainsi, dans les modules compare et merger, on peut continuer à utiliser mode=fm.READ ou mode=fm.WRITE.
Je l'ai testé, ça marche.

@aollier
Copy link
Contributor Author

aollier commented Dec 26, 2020

En plus, je dois de toute façon modifier le texte dans le fichier .md parce que les classes GzipFileManager etTextFileManager ne gèrent que l'ouverture de fichier, pas la fermeture.

@aollier
Copy link
Contributor Author

aollier commented Jan 4, 2021

Bonjour Thierry et bonne année ! 🎉 🎊
Est-ce que mes modifications actuelles te conviennent ou veux-tu que je fasse des modifications supplémentaires ?
Si tu es d'accord avec ce que j'ai fait, peux-tu valider ma requête de tirage s'il te plaît ?
Merci,
Adrien

@parmentelat parmentelat merged commit 1b4303d into flotpython:master Jan 15, 2021
@aollier aollier deleted the merger-gzip branch January 15, 2021 11:40
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.

Exercice ShipDict : l'option -z (--gzip) de merger.py ne fonctionne pas
2 participants