Rendere l'output di phicli json #3

blallo · 2020-11-20T12:12:46+01:00

blallo commented

2020-11-20 12:12:46 +01:00

Ho fatto tre modifiche in questa PR:

Ho aggiunto la possibilità di avere flag booleani (--flag/--no-flag, con default a False) dalla cli
Ho aggiunto un flag globale, con default a False per scegliere di mostrare le password nell'output dei comandi
Ho fatto in modo che l'output di showuser, adduser e showgroup sia del json valido, scegliendo in particolare di trasformare le password (che vengono fornite come bytes) nella lista di hex corrispondenti

Lungo la strada, ho anche applicato black. Non me ne vogliate. Mi mette l'anima in pace e ce l'ho direttamente integrato in vim.

Ho fatto tre modifiche in questa PR: - Ho aggiunto la possibilità di avere flag booleani (`--flag/--no-flag`, con default a `False`) dalla cli - Ho aggiunto un flag globale, con default a `False` per scegliere di mostrare le password nell'output dei comandi - Ho fatto in modo che l'output di `showuser`, `adduser` e `showgroup` sia del json valido, scegliendo in particolare di trasformare le password (che vengono fornite come `bytes`) nella lista di hex corrispondenti Lungo la strada, ho anche applicato [black](https://github.com/psf/black). Non me ne vogliate. Mi mette l'anima in pace e ce l'ho direttamente integrato in [vim](https://black.readthedocs.io/en/stable/editor_integration.html#vim).

crudo was assigned by blallo

2020-11-20 12:12:46 +01:00

uid was assigned by blallo

2020-11-20 12:12:46 +01:00

crudo requested changes 2020-11-20 15:33:09 +01:00

crudo left a comment

Ho svolto una prima revisione non completa.

Visto che la patch è formata in buona parte da modifiche estetiche, ho iniziato da quelle.

Il progetto era già lintato con flake8 e personalmente validavo ogni commit (cfr. flake8 --install-hook git). Non mi sembra il caso di usare due linter diversi.

Ho indicato senza ripetermi i punti in cui il linter ha fatto cazzate e dove si è comportato bene.

Per favore evitiamo di far scrivere sorgente alle macchine. Usare sempre, in ogni caso, senza possibilità di appello lo stesso stile non rende il codice più leggibile. Lasciare rifare a un software tutte le indentazioni in un modo standard (o cambiare tutte le virgolette) probabilmente impatterà positivamente sugli OCD di qualcuno ma IMHO non rende automagicamente il codice migliore o più chiaro.

Completerò la revisione più in là oggi stesso (spero).

Ho svolto una prima revisione **non** completa. Visto che la patch è formata in buona parte da modifiche estetiche, ho iniziato da quelle. Il progetto era già lintato con flake8 e personalmente validavo ogni commit (cfr. `flake8 --install-hook git`). Non mi sembra il caso di usare due linter diversi. Ho indicato senza ripetermi i punti in cui il linter ha fatto cazzate e dove si è comportato bene. Per favore evitiamo di far scrivere sorgente alle macchine. Usare sempre, in ogni caso, senza possibilità di appello lo stesso stile **non** rende il codice più leggibile. Lasciare rifare a un software tutte le indentazioni in un modo standard (o cambiare tutte le virgolette) probabilmente impatterà positivamente sugli OCD di qualcuno ma IMHO non rende automagicamente il codice migliore o più chiaro. Completerò la revisione più in là oggi stesso (spero).

src/phi/api/utils.py

						
				@ -4,3 +4,2 @@

				def serialize(d):

				    return {k: (v.isoformat() if isinstance(v, datetime) else v)

				            for k, v in d.items()}

				    return {k: (v.isoformat() if isinstance(v, datetime) else v) for k, v in d.items()}

crudo commented

2020-11-20 15:21:19 +01:00

Qui malissimo.

src/phi/app.py

						
				@ -20,3 +20,1 @@

				    web.run_app(app,

				                host=app['config']['core']['listen'].get('host', '127.0.0.1'),

				                port=app['config']['core']['listen'].get('port', '8080'))

				    web.run_app(

crudo commented

2020-11-20 15:21:05 +01:00

Qui male.

src/phi/cli.py

						
				@ -22,3 +23,3 @@

				        for i, name in enumerate(param_names):

				            info = param_infos[i] if i<len(param_infos) else ''

				            info = param_infos[i] if i < len(param_infos) else ""

crudo commented

2020-11-20 15:20:39 +01:00

Qui bene.

src/phi/config.py

						
				@ -42,3 +42,1 @@

				            raise FileNotFoundError("Could not find {} in any of {}."

				                                    .format(CONFIG_FILE,

				                                            ', '.join(CONFIG_PATHS)))

				            raise FileNotFoundError(

crudo commented

2020-11-20 15:21:36 +01:00

Qui male.

crudo commented

2020-11-20 15:35:23 +01:00

Inoltre non capisco perché l'unica regola a mio avviso sana, cioè di evitare righe più lunghe di 80 colonne, rispettata finora, è stata falciata via con violenza.

blallo commented

2020-11-20 16:11:03 +01:00

Inoltre non capisco perché l'unica regola a mio avviso sana, cioè di evitare righe più lunghe di 80 colonne, rispettata finora, è stata falciata via con violenza.

https://black.readthedocs.io/en/stable/the_black_code_style.html#line-length

Comunque è questione di scelte. Sappiamo che python ha ventuordicimila strumenti diversi per fare lint e fix. Ho trovato black il più facile da usare e per questo mi sono adattato.

Il progetto era già lintato con flake8 e personalmente validavo ogni commit (cfr. flake8 --install-hook git). Non mi sembra il caso di usare due linter diversi.

Perdonami, dove sta scritto? Non ho trovato riferimenti a questa convenzione di sviluppo nel readme o da altre parti.

> Inoltre non capisco perché l'unica regola a mio avviso sana, cioè di evitare righe più lunghe di 80 colonne, rispettata finora, è stata falciata via con violenza. https://black.readthedocs.io/en/stable/the_black_code_style.html#line-length Comunque è questione di scelte. Sappiamo che python ha ventuordicimila strumenti diversi per fare lint e fix. Ho trovato `black` il più facile da usare e per questo mi sono adattato. > Il progetto era già lintato con flake8 e personalmente validavo ogni commit (cfr. flake8 --install-hook git). Non mi sembra il caso di usare due linter diversi. Perdonami, dove sta scritto? Non ho trovato riferimenti a questa convenzione di sviluppo nel readme o da altre parti.

blallo commented

2020-11-26 18:56:46 +01:00

Cosa vogliamo fare con questa PR? Rejected? @crudo @uid

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin json-serialize-output:json-serialize-output

git checkout json-serialize-output

Merge

Merge the changes and update on Gitea.

git checkout master

git merge --no-ff json-serialize-output

git checkout master

git merge --ff-only json-serialize-output

git checkout json-serialize-output

git rebase master

git checkout master

git merge --no-ff json-serialize-output

git checkout master

git merge --squash json-serialize-output

git checkout master

git merge --ff-only json-serialize-output

git checkout master

git merge json-serialize-output

git push origin master

Sign in to join this conversation.

No reviewers

No Label

backend

cli

frontend

No Milestone

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: unit/phi#3