Skip to content

Conversation

@clementuu
Copy link
Contributor

Ajouter/supprimer/modifier les todos + connexion/inscription de l'utilisateur et stockage de ses info dans un fichier caché config.json

Copy link
Member

@hamdouni hamdouni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarde comment utiliser un LINTER pour GO dans ton éditeur pour capturer les "mauvaises pratiques" comme la non gestion d'erreur ou une écriture pas idiomatique

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

il y a moyen de mutualiser l'affichage de l'usage, par exemple, en créant une fonction "usage" et en l'utilisant partout où on en a besoin

attention au fait que le programme au final sera un binaire et pas utilisé à partir du code source : donc le "go run" ne convient pas. Remarque, il y a moyen de récupérer depuis le programme le nom du binaire exécuté (via le shell de mémoire)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cf remarque sur add.go pour une fonction "usage", pour y ajouter tous les usages (ici, la suppression)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

est ce que les templates de Go ne pourraient pas remplacer la génération de texte brut ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cela pourrait être la fonction "usage" dont j'ai parlé dans mes précédents commentaires

fmt.Println("priority : le niveau de priorité de votre tâche entre 1 et 3, du moins au plus urgent")
os.Exit(1)
}
id, err := strconv.Atoi(os.Args[2])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Il faudrait check l'erreur ici et mutualiser l'affichage de l'usage.
De plus, s'il y a une erreur de priorité, le programme continue tout de même son traitement ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c'est quoi ?

cli/user/user.go Outdated
func Login(configFilePath string) (u users.User, err error) {
configData, err := LoadConfig(configFilePath)
if err != nil {
fmt.Println(ERR_CFG, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C'est curieux d'afficher le message d'erreur à ce niveau, vu que la fonction est censée retourner une erreur. Mieux vaut laisser le soin de gérer l'erreur à l'appelant...
Même remarque dans le reste du code.

cli/user/user.go Outdated
}
var confirmmdp string
fmt.Print("Entrez votre email: ")
fmt.Scan(&configData.Email)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scan retourne une erreur

ihm/app.svelte Outdated
redirectTo("index.html");
} else if (statusCode == 500) {
alert(`${text}réessayez`);
} else if (statusCode == 401) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mieux vaut utiliser les codes http de Go que la valeur numérique

func Add(text Task, priority int, user users.User) (td Todo, err error) {
td.Task = text
if priority < 1 || priority > 3 {
err = fmt.Errorf("priorité de tâche invalide")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

peut être écrit : return td, fmt.Errorf("blabla")

…rme, code plus idiomatique sur l'ensemble du projet
…rité + binaire mytodolist et install.sh pour copier le binaire sous usr/local/bin et pouvoir l'executer depuis n'importe quel emplacement
…ur pouvoir executer l'app cli depuis n'importe quel emplacement
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