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

Optimize file generation and overloading #281

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

jocgir
Copy link
Contributor

@jocgir jocgir commented Dec 3, 2024

  • Fix file generation to ensure that we don't create useless files or rename files that have not been modified
  • Add a function Diff on String to be able to view the diff between two strings
  • Remove some spelling errors

- Fix file generation to ensure that we don't create useless files or rename files that have not been modified
- Add a function Diff on String to be able to view the diff between two strings
- Remove some spelling errors
@jocgir jocgir requested a review from a team as a code owner December 3, 2024 13:04
Comment on lines 255 to 256
fmt.Println("error", err)
InternalLog.Debugf("%s(%d): Execution error %v", th.Filename, th.Try, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

c'est un doublon dans les logs si debug est on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Thanks

@@ -279,5 +278,6 @@ func (t *Template) processContentInternal(originalContent, source string, origin
}
result = s.Str()
}
changed = result != originalContent
Copy link
Contributor

Choose a reason for hiding this comment

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

J'imagine que c'est setté pour les defer... en particulier le premier?

Avec 2 defers (un des 2 va setter changed à false) ça feel comme un code smell, aggravé par la longueur de la fonction. Je ne suis pas certain comment améliorer cela mais j'ai l'impression que l'ordre d'invocation des defers est important et c'est facile à casser?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je suis d'accord pour ce qui est de la longueur de la fonction. Ça serait à améliorer.

Mais non, je ne crois pas que ça casse la fonction. Les 2 defers sont exécutés dans des contexts différents. Dans le premier cas, c'est si on applique un manual handler qui apporte une modification après coup. Il est exécuté seulement après l'évaluation de la fonction.

Dans le second defer, le but est de générer une erreur si des variables ne sont pas définies (selon le mode d'opération de gotemplate). Si on rapporte une erreur, alors, on ne considère pas qu'il y a eu des changements.

Copy link
Contributor Author

@jocgir jocgir Dec 3, 2024

Choose a reason for hiding this comment

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

Et non, la variable changed n'est pas settée pour les defers, c'est vraiment le retour de la fonction. Quand on utilise des named return, on n'a pas besoin de retourner les variables sur le return.

Et les defers peuvent également changer le résultat de la fonction (après le return).

Copy link
Contributor

Choose a reason for hiding this comment

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

aaah les maudits named return... Belle invention du diable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Reste que là, on a juste 1 defer qui peut changer le changed. Si on en avait 2, ça commencerait à être stressant :P

@jocgir jocgir merged commit 3f6b7dd into main Dec 3, 2024
3 checks passed
@jocgir jocgir deleted the bugs/DT-7519-do_not_generate_files_if_no_change branch December 3, 2024 13:58
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