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

Use async_forecast and manage hourly&daily forecasts on same card. #130

Merged
merged 1 commit into from
Jan 7, 2024

Conversation

manu3b1
Copy link

@manu3b1 manu3b1 commented Jan 4, 2024

Fix #129
Rework Card Editor: French translation, paper-input deprecated, reordering
Rework Card: Default City name deduced from Entity

@dx44
Copy link
Member

dx44 commented Jan 4, 2024

Vu que le hourly et Daily sont maintenant utilisables simultanément, je te propose d'adapter le code pour identifier plus clairement la config autour du daily :

get _forecast()

en

get _daily_forcast()

Et donc les utilisations de cette méthode dans tout le code.

Sinon merci pour ton job, top 👍🏻 🤩

@manu3b1
Copy link
Author

manu3b1 commented Jan 4, 2024

Bien vu, c'était pour voir si tu avais bien relu ;-)
En fait, j'ai essayé de garder la compatibilité avec la config existante et ne faire que "rajouter" de nouveaux paramètres.
J'imagine qu'on veut que lors de la mise à jour, tout marche comme avant pour les utilisateurs. Ils auront juste plus de choix.
A moins qu'on puisse "upgrader" une config?

Autre question: j'ai mis en commentaire l'affichage du champ "Name" car on peut le déduire du "friendly_name". Mais il est toujours dispo en mode YAML.
Idem pour "Icons location", car je le trouve pas très utile pour le End User.
Ca te va?

Je dois mettre à jour le README/info et CHANGELOG?

@dx44
Copy link
Member

dx44 commented Jan 5, 2024

Hehe je relis toujours et j'essaie de comprendre 😅

Je ne sais pas si il existe une notion d'upgrade dans les méthodes HA, c'est à chacun de prévoir à mon sens. En soit je suis d'accord avec toi et il n'y a pas de nécessité à modifier hormis la lisibilité. Donc laisse comme tu as fait 👍🏻

Pour les champs Name et Icon, et pour reprendre ta remarque précédente, autant les laisser personnalisable, certains utilisateurs les ont peut-être adapté, donc pas de breaking change.

Tu peux mettre à jour le README sans problème et au contraire 😄 et je m'occuperai du CHANGELOG à la publication de la version 😉

@manu3b1
Copy link
Author

manu3b1 commented Jan 6, 2024

J'ai trouvé comment gérer l'upgrade de la config:

  // Upgrade config fields if necessary
  upgradeConfig(config) {
    const upgradedConfig = { ...config }
    // Deduce "daily_forecast" from deprecated "forecast"
    if (config["forecast"] !== undefined && config["daily_forecast"] === undefined) {
      upgradedConfig["daily_forecast"] = config["forecast"];
    }
    return upgradedConfig;
  }

Et j'ai cleané l'init de la carte qui se passait très mal (il fallait reselectionner une entité): tous les champs sont maintenant pré-remplis à l'ouverture de la carte.

Bon, maintenant va falloir stresser un peu tout ca.
Comment ca se passe: il y a beaucoup de personnes qui testent les beta?

Rework Card Editor: French translation, paper-input deprecated, reordering, all fields defaulted

Signed-off-by: Emmanuel Berthier <[email protected]>
@dx44 dx44 changed the base branch from dev to beta January 7, 2024 10:50
Copy link
Member

@dx44 dx44 left a comment

Choose a reason for hiding this comment

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

LGTM, go pour la beta !

@dx44
Copy link
Member

dx44 commented Jan 7, 2024

Pour les tests je vais publier dans le forum le nouveau tags de cette beta, après il n'y a pas de groupe de personne spécifique, on prend les bonnes volontés 😄

@dx44 dx44 merged commit b9aa9ae into hacf-fr:beta Jan 7, 2024
@manu3b1
Copy link
Author

manu3b1 commented Jan 7, 2024

Extra!
Ok, tu me donneras le lien vers le post, je surveillerai les retours.
Les screenshots ne sont pas visibles depuis le README car les liens pointent sur la branche Meteo-france, faudra les mettre dans le post pour qu'ils aient une idée du rendu.

J'ai l'impression que HACS montre le info.md et pas le README.md. Faut recopier l'un dans l'autre?

@dx44
Copy link
Member

dx44 commented Jan 7, 2024

J'ai fait une pre-release et il y a un soucis avec la migration depuis l'ancien code, rien de grave mais le hourly et daily sont actif dans la carte alors que l'on a l'un ou l'autre auparavant.
Le plus simple sera que je le précise en breaking change.

@manu3b1
Copy link
Author

manu3b1 commented Jan 7, 2024

Effectivement, je vais regarder ca.

@manu3b1
Copy link
Author

manu3b1 commented Jan 7, 2024

@dx44
Copy link
Member

dx44 commented Jan 8, 2024

Yes pour la PR 👍🏻

@manu3b1
Copy link
Author

manu3b1 commented Jan 8, 2024

Dans la note de release 1.9.2-beta-1, il manque les 2 evolutions majeures, celles du titre du patch:

  • Use async_forecast
  • Manage hourly&daily forecasts on same card.

;-)
J'ai poussé le PR de la 2eme correction, j'en ai profité pour recopier le README.md dans le info.md et pointer sur les PNG locaux: on verra les images à jour dans toutes les branches.

@manu3b1
Copy link
Author

manu3b1 commented Feb 27, 2024

Hello @dx44, on release la beta?

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