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

Deprecate unused variables #671

Merged
merged 7 commits into from
Jan 30, 2017
Merged

Deprecate unused variables #671

merged 7 commits into from
Jan 30, 2017

Conversation

benjello
Copy link
Member

No description provided.

@benjello benjello requested a review from fpagnoux January 26, 2017 15:22
@fpagnoux fpagnoux changed the title Deprecate unuised variables Deprecate unused variables Jan 26, 2017
@@ -7,7 +7,7 @@

setup(
name = 'OpenFisca-France',
version = '12.0.4',
version = '12.0.5',
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it a major change, to remove 2 variables?

@MattiSG any advice?

Copy link
Member

Choose a reason for hiding this comment

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

En théorie, oui.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ce sont des reliquats qui ont été remplacés par d'autres variables sans avoir été supprimées.
C'est plus du nettoyage qu'autre chose.

Copy link
Member

Choose a reason for hiding this comment

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

Dépréciation = changement mineur à minima.
Suppression = changement majeur.

Cf. https://speakerdeck.com/mattisg/git-session-2-strategies?slide=81

@fpagnoux
Copy link
Member

fpagnoux commented Jan 26, 2017

Pour bien comprendre :
Pourquoi ces variables ont-elles été crées si elles ne sont utilisées nulle part ? Est-ce qu'elles étaient avant appelées par d'autres formules ? Est-ce que c'est des doublons ?

## 12.0.5

* Minor version change because deprecating unused variables
* Deprecate nbsala
Copy link
Member

Choose a reason for hiding this comment

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

s/deprecate/delete (?)

Copy link
Member

Choose a reason for hiding this comment

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

Jusqu'à maintenant on utilise plutôt deprecate.

Copy link
Member

Choose a reason for hiding this comment

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

Well 👌

Copy link
Member Author

Choose a reason for hiding this comment

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

@benjello
Copy link
Member Author

benjello commented Jan 26, 2017 via email

@MattiSG
Copy link
Member

MattiSG commented Jan 26, 2017

Pour moi c'est un changement mineur.

Il n'y a pas à avoir d'évaluation personnelle : le schéma de versionnement est totalement normalisé et documenté sur http://semver.org :)

S'il y a une suppression, cela change l'API publique de manière non rétro-compatible, et cela occasionne donc un changement du numéro de version majeur.

@benjello
Copy link
Member Author

@fpagnoux @cbenz is this one GTM


* Deprecate `nbsala`
* Deprecate `tva_ent`
* Low impact changes since deprecating unused variables
Copy link
Member

@fpagnoux fpagnoux Jan 30, 2017

Choose a reason for hiding this comment

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

When I read this, I understand that you deprecated two variables and did other low impact changes.

I'm not even sure the third point is necessary at all. If you think it is important, please make sure to explain that is it not a change, but additional info about the two first lines.

For instance you can remove the bullet, and instead having a full sentence "These changes are low impact since the two deprecated variables were not used"

Copy link
Member

@fpagnoux fpagnoux left a comment

Choose a reason for hiding this comment

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

See Changelog

@benjello
Copy link
Member Author

@fpagnoux is this GTM for you now ?

@benjello benjello merged commit 04355b1 into master Jan 30, 2017
@benjello benjello deleted the deprecate-unuised-variables branch January 30, 2017 11:03
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.

4 participants