-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
[frenchgovtenergydata] Initial contribution #16713
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just did a quickscan, the readme needs some attention, just like the pom.xml (newlines)
@clinique : you have a problem with your file bundles/pom.xml. As already mentioned, README file has to be completed. I will review your PR. |
...penhab.binding.edf/src/main/java/org/openhab/binding/edf/internal/handler/TariffHandler.java
Outdated
Show resolved
Hide resolved
...penhab.binding.edf/src/main/java/org/openhab/binding/edf/internal/handler/TariffHandler.java
Outdated
Show resolved
Hide resolved
...penhab.binding.edf/src/main/java/org/openhab/binding/edf/internal/handler/TariffHandler.java
Outdated
Show resolved
Hide resolved
...penhab.binding.edf/src/main/java/org/openhab/binding/edf/internal/handler/TariffHandler.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.edf/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.edf/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.edf/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.edf/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.edf/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
...ab.binding.edf/src/main/java/org/openhab/binding/edf/internal/handler/BaseTariffHandler.java
Outdated
Show resolved
Hide resolved
@jlaur already created the "Energi Data Service Binding" for Danish prices. |
Good idea. I do not like the idea of having "edf" providing only this. "Energi Data Service" could become "Energy Data Services" having multiple countries and multiple providers by country. |
Yes, that is an option. To be discussed with @jlaur. |
Main difference being that in many countries prices can vary a lot, while in France, base is regulated and changes only once or twice a year. |
bundles/org.openhab.binding.edf/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.edf/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.edf/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
Lets think to a different binding name, even if we can keep your current description mentioning EDF. |
Renaming a whole binding is not a small change...You kill me. It makes sense. Let's try to do this. |
First we decide a good name ... before you change everything. |
Maybe "French Electricity Tariffs" |
Let's vote for this |
One remaining comment that I would appreciate if you could consider, and then we can merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you
I do not agree with this, and in my opinion "French Electricity Tariffs" is also too generic. This is for the same reason you wouldn't merge the Miele and Home Connect bindings which both implement home appliance integrations, but for different vendors and protocols. Energi Data Service is a specific service. There is a different service available, Eloverblik, which overlaps with EDS in the data provided, but also provides data for energy meters (needs authentication of course). If I were to implement that service, I would also do that in a new binding, |
I had a brief look at https://www.data.gouv.fr, and it looks quite similar to Energi Data Service since it provides other data like gas/fuel prices as well. So I think the name is both too generic and too specialized. You wouldn't be able to add support for gas prices with a name like "French Electricity Tariffs", at least it would seem a bit strange? So that would be another binding, "French Gas Prices" implementing basically the same API? What is "EDF" an abbreviation for? This is just my opinion, and @lolodomo can merge as is, if you both agree on the current name, I just wanted to add my input to the discussion, although you are moving fast. 😉 |
Maybe "French Energy Data Service" ? It is a long name... @clinique : WDYT ? |
I still think it's misleading unless that's actually the name of the service? It's too generic, there could be other energy-related services in France. Hence:
Is there a full name that would actually fully represent the service being implemented?
That's a different issue, i.e. the modelling. In EDS binding I used channel groups, so first one is electricity. I didn't see a need to have different Things for the same service, so I went for channel groups. But of course there are multiple ways to structure/model this. |
French Electricity Tariffs was too much specific while French Energy Data Service is too much generic ? EDF is the very big company in France that manages electricity in France and the main commercial provider. What provides this binding today is regulated electricity tariffs only provided by EDF. These tariffs are negociated by EDF with the French government. |
Maybe "French Electricity Regulated Tariffs" if the goal is to have something very specialized not open to other energy tariffs in the future ? |
No, I think both are too generic, but French Electricity Tariffs is also too specialized. French Electricity Tariffs is too generic because electricity tariffs could be provided by different services, and here the concept/term is used as name rather than the actual/specific service implemented by the binding. And at the same time it's too specialized because it doesn't leave room for other types of data provided by data.gouv.fr - like gas. French Energy Data Service can include gas and other types of data, but it seems somewhat made up - I mean, it's you calling/naming it an energy data service, right, not EDF? It seems similar to the Danish Energi Data Service, but in the Danish case Energi Data Service is actually the name of the service:
I'm also a little lost. But EDF seems to be the French equivalent to Energinet in Denmark, who is in charge of the grid, and also is the company to provide Energi Data Service. I think the service in your case is actually named data.gouv.fr? And it's operated by EDF, just like Energi Data Service is operated by Energinet?
Is it correctly understood? And is this the logo that could also be used for the binding? EDIT: Or perhaps: |
data.gouv.fr is provided by the French government, not EDF. And it provides many different data. |
OK. But this is the service implemented by this binding, correct? |
My assumption is based on this code line: private static final String URL = "https://www.data.gouv.fr/fr/datasets/r/%s"; |
data.gouv.fr is kind of portal to deliver many data. |
French Govt Energy Data ? |
Even if it provides only a subset of the available data, wouldn't it still make sense to call it like that, and if there would be any reason to support other datasets, it could be done in context of this binding? Also think which logo you would use, it might give some kind of direction. If I would implement another binding for a different API related to French energy data, how would I choose a different name for such a binding? That's part of the reason why I think French Energy Data Service is too generic. I don't think I have much more to offer. I hope I didn't annoy you too much, just wanted to chip in with another perspective, since you asked me yesterday, and I only got back today. 🙂 |
Btw, I think this is quite similar to Energi Data Service: https://www.energidataservice.dk/datasets. Most recently I added #16330. |
"French Govt Energy Data" shows that it is coming from a French government source and also restricts the binding to energy data. |
Or we call it "French Govt Data Service" if we expect to provide other data from data.gouv.fr in the future in that binding. Why not. |
Okay, so I get it, data.gouv.fr also provides data for agriculture, for example, so it's even wider than Energi Data Service in Denmark. It would probably be reasonable to limit the scope to the energy-related datasets. "data.gouv.fr Energy"? Oh man, this is hard, why did they choose a domain as service name? 😄 I think we are getting closer though. |
"French Govt Energy Data" looks good. |
If we go this way, I think I must also change thing IDs to include 'electricity' |
@clinique :let's keep it specialized to energy => French Govt Energy Data But yes thing types should clearly mention electricity. And mention data.gouv.fr in the binding description. |
Signed-off-by: Gaël L'hopital <gael@lhopital.org>
Signed-off-by: Gaël L'hopital <gael@lhopital.org>
...les/org.openhab.binding.frenchgovtenergydata/src/main/resources/OH-INF/thing/thing-types.xml
Outdated
Show resolved
Hide resolved
Signed-off-by: clinique <gael@lhopital.org>
@clinique: I suggest to replace "French Govt Energy Data Binding" by "French Government Energy Data Binding" at the five places it is used (POM, README, feature, addon.xml and properties file), but keeping frenchgovtenergydata as binding name. WDYT ? |
Thank you Gael. |
This new binding provides electrical energy prices for the french historical provider EDF.