Re: KDE review for KWeatherCore
I've applied the following changes: * Remove @brief * DailyWeatherForecast and HourlyWeatherForecast constructor now take one QDate/QDateTime arguement. The previous "one for all" constructor has been removed * various setters now take const reference * other naming, misc changes Regards, Han
Re: KDE review for KWeatherCore
Hi Han, let's have this discussion public on kde-core-devel and kde-framworks, so other can contribute. On Sat, Jan 2, 2021 at 1:29 PM hanyoung wrote: > > Hi Dominik, > > Thank you for your time. This is my first time writing a library, I tried my > best by looking > at existing KDE frameworks source code and articles on KDE wiki. Obvious > KWeatherCore > is far from polished. However, there are still something I want to explain. There is always a first time, so welcome :-) > > > - #pragma once: It's used in all header files. > I've once encountered name conflict with one KDE frameworks and that took me > half a hour > to debug. I want to #pragma once to avoid this kind of issue to appear, since > most modern > compiler supports this feature. Yes, I am aware that essentially all compilers support it. I was just mentioning the status quo. > > > 1. The first sentence in doxygen is automatically he brief version. > It's on QHC documentation, so I just copied it. I guess it's fine since it > doesn't do any harm. It does harm in the sense that it adds visual noise. That is why I suggested to remove it. Other example: Many developers (outside of KDE) use void parameters such as in void foobar(void); This (void) is not needed either, it just adds noise. It's the exact same thing here with doxygen. You don't need it. So why add it? > > > 2. Obviously, GeoTimezone is not a complete documentation :-) > Can't argue with that :P > Actually, some problems with this class are all partially related to this. > > > 5. Copyright: > Didn't know about that, my apologies. Will definitely change that. > > > 6. File naming. > Didn't notice that. My bad. > > > 7. Constructors. > It has another constructor takes zero arguments. And this class provides all > setters and getters. > The reason this "one for all" constructor exists is for internal convenience > use. And I thought > export it does no harm. Let me try to explain this a bit more in depth. When back in the old days Trolltech released Qt4, they broke a lot of API. One reason for breaking API was the focus on good and readable API. In fact, Mathias Ettrich even held a talk to the KDE audience in Ludwigsburg, Germany in 2004 why API design matters a lot. The link I mentioned previously (https://doc.qt.io/archives/qq/qq13-apis.html) essentially contains all the points back from this talk. Qt tries to stick to these rules, and KDE does as well. Therefore I'd like to emphasize this again: Constructors that take e.g. 8 arguments and most of them are of type double will lead to unreadable code. Theses constructors also will not work, once you extend the class with function and member. Then, you have one missing member that is not included in the constructor. So this approach is not open to extensions. Or it short: that is bad API design :-) Let me phase it differently: Can we find an API that is even better? You have a default constructor - fair enough. But I believe it can be better, that is why I suggested the version that takes only a QDate, since you cannot misuse it, and the API is rather explicit. And the most important property of a forecast is the date, since otherwise the information is useless. > > 8. Wrong order of API documentation: > My bad. > > > 9. Pass non primitive data types (pod) by const reference: > I use std::move, so pass by value means one copy and one move for lvalue, one > move for rvalue. > Const reference costs one copy for everything. Pass by value saves me one > rvalue reference overload function. > Consider that I have so many setters, it's justifiable. I saw and understood this, yes. But this approach still has its drawbacks. Let me explain. When you use std::move internally, then you don't have any performance penalty for now, since the version with const ref would also do a copy later. But let's say you need to internally change the implementation such that you don't need a copy anymore. Instead you just read this parameter. Then, you still have the copy in the public API and you cannot change it anymore due to binary compatibility. Or in other words: Currently you force API design decisions on the public API that rely on some internal implementation details. That is always a bad idea. I hope what I wrote is understandable :-) > > 10. Mixup of documentation + unexport macro. > I can definitely improve that. > > > I assume the weatherDescription is a possibly user visible string? > Thank you, I just noticed that default value for weatherIcon and > weatherDescription are reversed. My bad. > > Thanks again for taking time to look through my terrible code base. It's not terrible, it just needs several rounds of reviews to reach the quality to be included in kde-frameworks. > Happy new year! Thanks, same to you! Best regards Dominik
Re: KDE review for KWeatherCore
On Samstag, 2. Januar 2021 12:44:31 CET Dominik Haumann wrote: > This is just by looking at the first two header files. > > Looking at WeatherForecast.cpp: > > double maxTemp = std::numeric_limits::min(); > double minTemp = std::numeric_limits::max(); > > Initializing the temperature to 0, 0, sounds a bit more sane to me, but > that is disputable I guess. Nice find. This looks quite familiar, the weather forecast accumulation code in Itinerary does something very much like this, to make std::min/std::max work without special-casing invalid values. However the way it's done here will fail for negative max temperatures, as std::numeric_limits::min() is not what one might expect (it's the smallest positive value for floating point types), you want std::numeric_limits::lowest(). Regards, Volker signature.asc Description: This is a digitally signed message part.
Re: KDE review for KWeatherCore
Hi Han,
just having a quick glance at the code, and I feel like there is a lot of
polishing
missing. I'll give some examples, but this is by no means a complete review,
and I'll not comment on technical aspects at all, since this is not the
domain
of my expertise.
- #pragma once: It's used in all header files. I am not sure whether we have
any policy here, but indeed it's almost not used at all in KDE Frameworks.
Maybe someone else can clarify.
- API documentation largely incomplete: For instance, looking at
geotimezone.h,
we see
/**
* @brief GeoTimezone
* @param lat latitude
* @param lon longitude
* @param parent
*/
GeoTimezone(double lat, double lon, QObject *parent = nullptr);
1. The first sentence in doxygen is automatically he brief version. You can
omit @brief
everywhere. Its just noise.
2. Obviously, GeoTimezone is not a complete documentation :-)
3. Try to avoid abbreviations. That is: Name the parameters latitude and
longitude
instead of lat and lon. Explaining a parameter with the name itself is also
no good
practice. Better would be: @param latitude The latitude used in
geographical timezone. or so...
Q_SIGNALS:
/**
* @brief finished
* @param timezone
*/
void finished(const QString );
4. What is finished? It seems to me that this API is not intuitive :-)
And again: the API documentation is essentially missing.
/**
* @brief networkError encounted network error
*/
void networkError();
Better would be networkErrorOccured() or so, to stress the fact that this
is a signal.
5. Copyright:
* Copyright 2020 Han Young
* Copyright 2020 Devin Lin
* SPDX-License-Identifier: LGPL-2.0-or-later
I think nowadays we encourange using SPDX-FileCopyrightText to list the
author.
This can appear multiple times, from what I understand.
https://community.kde.org/Policies/Licensing_Policy#SPDX_Statements
6. File naming.
We have dailyforecast.h, but the class is called DailyWeatherForecast.
It's good practice to name the header files exactly like the class names
for public API.
7. Constructors. Let's have a look at this:
DailyWeatherForecast(double maxTemp,
double minTemp,
double precipitation,
double uvIndex,
double humidity,
double pressure,
QString weatherIcon,
QString weatherDescription,
QDate date);
Using this looks like this: DailyWheaterFoecast forecast(0, 10, 35, 80,
1023, "sun", "Sunny Weather", ...);
Please read "The Convenience Trap" here:
https://doc.qt.io/archives/qq/qq13-apis.html
In other words: Try to avoid constructors that take many many arguments to
define the object in one go.
What is much more readable is to have 3-4 lines more code, but at least it
can be understood:
DailyWheterhForecast forecast(QDate(2020, 12, 31));
forecast->setIcon("sun");
forecast->setDescription("Nice beach weather");
forecast->setTemperatureRange(2, 27);
...
8. Wrong order of API documentation:
/**
* Return a QJsonObject that can be converted back with
* DailyWeatherForecast::fromJson
*/
~DailyWeatherForecast();
QJsonObject toJson();
Here, the virtual destructor will get the API documentation of toJson().
Obviously not what is wanted.
9. Pass non primitive data types (pod) by const reference:
void setWeatherIcon(QString icon);
void setWeatherDescription(QString description);
void setDate(QDate date);
const QString &
const QDate &
10. Mixup of documentation + unexport macro.
/**
* @brief return maximum temperature
* @return maximum temperature, this value is initialized to the
smallest
* value double can hold
*/
KWEATHERCORE_NO_EXPORT void setJsDate(const QDateTime );
double maxTemp() const;
Here, setJsDate() will get the API documentation of maxTemp(). That is not
what you want.
Typically, having to explicitly hide a symbol (_NO_EXPORT) is showing
design issues.
I'd recommend to move this function to the pimpl class, and if you need
internally access to
this function, then move your pimpl class to e.g. dailyweatherforecast_p.h.
This is just by looking at the first two header files.
Looking at WeatherForecast.cpp:
double maxTemp = std::numeric_limits::min();
double minTemp = std::numeric_limits::max();
Initializing the temperature to 0, 0, sounds a bit more sane to me, but
that is disputable I guess.
QDate date = QDate();
The default constructor will be called automatically, better would be
simply:
QDate date;
bool isNull(); The semantics of isNull() is a bit weird. Typically, the
naming of bool isValid() is
used in Qt and therefore better.
I assume the weatherDescription is a possibly user visible string?
Currently, this is initialized
to QString weatherDescription =
QStringLiteral("weather-none-available"); So possibly
the
Re: KDE review for KWeatherCore
I've made some changes to KWeatherCore according to the feedback. Namely: * More Private Classes * Removed inline functions * Lowered coordinates resolution * Listed API services in use on README * Switched to LGPL Regards, Han
Re: KDE review for KWeatherCore
On lundi 21 décembre 2020 07:16:09 CET hanyoung wrote: > KWeatherCore: https://invent.kde.org/libraries/kweathercore is a library for > querying weather forecast data. During the development of KWeather, we > found the need to have a weather library. KWeatherCore is the result of > extracting weather data fetching code from KWeather. I think having a > dedicated weather library can serve the following propose: - simplify the > KWeather code > - easier to develop a weather daemon > - potentially less code duplication across KDE Very interesting. I've been missing such a library 4 years ago when I wrote a QML app to display the weather on a RPi. https://github.com/dfaure/qrpiweather I remember looking at the plasma weather-engine but most of the data sources didn't have information about wind, and wind is very very important where I live (it's actually blowing up to 105 km/h at the very moment) :) Feel free to grab any code from qrpiweather that might be useful, BTW. The backend I ended up using was "Open Weather" (api.openweathermap.org). But well, that was 4 years ago, things might have changed since :) -- David Faure, fa...@kde.org, http://www.davidfaure.fr Working on KDE Frameworks 5
Re: KDE review for KWeatherCore
El dilluns, 21 de desembre de 2020, a les 7:16:09 CET, hanyoung va escriure: > KWeatherCore: https://invent.kde.org/libraries/kweathercore is a library for > querying weather forecast data. > During the development of KWeather, we found the need to have a weather > library. > KWeatherCore is the result of extracting weather data fetching code from > KWeather. > I think having a dedicated weather library can serve the following propose: > - simplify the KWeather code > - easier to develop a weather daemon > - potentially less code duplication across KDE There's quite some things that need improvement: * You don't have d-pointer in most of the public classes * You have quite some code inline in the .h which makes keeping BC harder * You return const Q* & which is not usual in Qt classes locationquerytest is unstable https://paste.debian.net/1177864/ You have a script that extracts translations to libkweather5 but then you do -DTRANSLATION_DOMAIN=\"kweathercore5\" Cheers, Albert > > Many of you may have already seen my previous email to kde-devel mailing list. > Thank you for your constructive suggestions. Here are something I want to > clarify: > > > I would also propose to consider doing a demon instead, so different > > programs/processes all interested in weather forecast data could share the > > data > The end goal is a daemon indeed, but we want to build the daemon upon the > library. This would give us flexibility > in the future if we don't want a daemon. At least KWeather and other > projects can still benefit from the code. > > > but we want to make sure we don't end up with two things. > I admit there are some overlapped functionalities. But KWeatherCore isn't > designed as a weather data provider as Weather DataEngine. > Instead, it's trying to be the building block of weather related > applications. KWeatherCore saves you the hassle of > dealing with APIs, getting locations and converting timezone. You can build > a daemon with it, or you can > use it in your applications. For example, KItinerary and KWeather use the > same weather API, but don't share code. > That means two code base to maintain. Regarding the dynamic nature of > online APIs, it's better to have one library, > so other applications don't need to be worried about their APIs being > depraved, and they aren't able to update it in time. > > Though not currently implemented, KWeatherCore does intend to have weather > alerts added. We hope it can be done in this Sok > https://community.kde.org/SoK/Ideas/2021#KWeather > With this bit added, then the work on weather daemon can be started. > > Regards, > Han Young > >
Re: KDE review for KWeatherCore
Am Montag, 21. Dezember 2020, 15:45:22 CET schrieb Nicolas Fella: > On 12/21/20 3:19 PM, Friedrich W. H. Kossebau wrote: > > My idea/proposal there is that the library internally makes use of that > > demon. So code which uses KWeatherCore does not need to know that > > implementation-wise there is a demon (which might also need to be a > > build-time option, think app bundles who do not like separate demon > > processes). > > So the demon would not use KWeatherCore, but be a(n optional) backend part > > of it. > > Please keep in mind that having such a daemon would be challenging to > impossible to implement on Android and possibly other platforms as well. That's what I tried to hint at with "(which might also need to be a build-time option, think app bundles who do not like separate demon processes)." > Let's not overengineer the wheel without having to and focus on use > cases relevant for our current apps and less on hypothetical use cases. A demon is not overengineered for this purpose IMHO, and I had thought quite a bit how to overhaul the Plasma weather dataengine to make it sanely reusable as system-wide service, just never got to the editor to implement things. But those who do the code decide and can apply their favourite architectures and whatever makes reaching a working product for them faster :) Cheers Friedrich
Re: KDE review for KWeatherCore
On 12/21/20 3:19 PM, Friedrich W. H. Kossebau wrote: Am Montag, 21. Dezember 2020, 07:16:09 CET schrieb hanyoung: KWeatherCore: https://invent.kde.org/libraries/kweathercore is a library for querying weather forecast data. During the development of KWeather, we found the need to have a weather library. KWeatherCore is the result of extracting weather data fetching code from KWeather. I think having a dedicated weather library can serve the following propose: - simplify the KWeather code - easier to develop a weather daemon - potentially less code duplication across KDE Many of you may have already seen my previous email to kde-devel mailing list. Thank you for your constructive suggestions. Here are something I want to clarify: I would also propose to consider doing a demon instead, so different programs/processes all interested in weather forecast data could share the data The end goal is a daemon indeed, but we want to build the daemon upon the library. This would give us flexibility in the future if we don't want a daemon. At least KWeather and other projects can still benefit from the code. My idea/proposal there is that the library internally makes use of that demon. So code which uses KWeatherCore does not need to know that implementation-wise there is a demon (which might also need to be a build-time option, think app bundles who do not like separate demon processes). So the demon would not use KWeatherCore, but be a(n optional) backend part of it. Please keep in mind that having such a daemon would be challenging to impossible to implement on Android and possibly other platforms as well. Let's not overengineer the wheel without having to and focus on use cases relevant for our current apps and less on hypothetical use cases. Cheers Nico
Re: KDE review for KWeatherCore
Would it be possible for KweatherCore to lean on location services like Geoclue? (And/if should we be working on a KDE GUI for Geoclue to help with this? That way Kweather could only be dealing with resolving weather info) On Sunday, December 20, 2020 10:16:09 PM PST hanyoung wrote: > KWeatherCore: https://invent.kde.org/libraries/kweathercore is a library for > querying weather forecast data. During the development of KWeather, we > found the need to have a weather library. KWeatherCore is the result of > extracting weather data fetching code from KWeather. I think having a > dedicated weather library can serve the following propose: - simplify the > KWeather code > - easier to develop a weather daemon > - potentially less code duplication across KDE > > Many of you may have already seen my previous email to kde-devel mailing > list. > Thank you for your constructive suggestions. Here are something I want to clarify: > > I would also propose to consider doing a demon instead, so different > > programs/processes all interested in weather forecast data could share the > > data > > The end goal is a daemon indeed, but we want to build the daemon upon the > library. This would give us flexibility in the future if we don't want a > daemon. At least KWeather and other projects can still benefit from the > code. > > but we want to make sure we don't end up with two things. > > I admit there are some overlapped functionalities. But KWeatherCore isn't > designed as a weather data provider as Weather DataEngine. Instead, it's > trying to be the building block of weather related applications. > KWeatherCore saves you the hassle of dealing with APIs, getting locations > and converting timezone. You can build a daemon with it, or you can use it > in your applications. For example, KItinerary and KWeather use the same > weather API, but don't share code. That means two code base to maintain. > Regarding the dynamic nature of online APIs, it's better to have one > library, so other applications don't need to be worried about their APIs > being depraved, and they aren't able to update it in time. > > Though not currently implemented, KWeatherCore does intend to have weather > alerts added. We hope it can be done in this Sok > https://community.kde.org/SoK/Ideas/2021#KWeather > With this bit added, then the work on weather daemon can be started. > > Regards, > Han Young signature.asc Description: This is a digitally signed message part.
Re: KDE review for KWeatherCore
Am Montag, 21. Dezember 2020, 07:16:09 CET schrieb hanyoung: > KWeatherCore: https://invent.kde.org/libraries/kweathercore is a library for > querying weather forecast data. During the development of KWeather, we > found the need to have a weather library. KWeatherCore is the result of > extracting weather data fetching code from KWeather. I think having a > dedicated weather library can serve the following propose: - simplify the > KWeather code > - easier to develop a weather daemon > - potentially less code duplication across KDE > > Many of you may have already seen my previous email to kde-devel mailing > list. > Thank you for your constructive suggestions. Here are something I want to clarify: > > I would also propose to consider doing a demon instead, so different > > programs/processes all interested in weather forecast data could share the > > data > > The end goal is a daemon indeed, but we want to build the daemon upon the > library. This would give us flexibility in the future if we don't want a > daemon. At least KWeather and other projects can still benefit from the > code. My idea/proposal there is that the library internally makes use of that demon. So code which uses KWeatherCore does not need to know that implementation-wise there is a demon (which might also need to be a build-time option, think app bundles who do not like separate demon processes). So the demon would not use KWeatherCore, but be a(n optional) backend part of it. To give you more ideas: I would also envision there could be proxy servers one day. Think of big organizations with lots of devices at same location due to lots of people in same buildings, so interested in the same local weather data, like schools, office-heavy companies, they could have a single proxy server and all clients would just use that proxy, saving the weather/ environment service provider some bandwidth, also adding some privacy layer onto the provider. If KWeatherCore would be prepared to handle that scenario in one way or the other, even better. > > but we want to make sure we don't end up with two things. > I admit there are some overlapped functionalities. But KWeatherCore isn't > designed as a weather data provider as Weather DataEngine. Instead, it's > trying to be the building block of weather related applications. Consider a DataEngine to be some kind of Plasma-specific type of library, as in, as shared logic module, just with a normalized API. This technology though failed to stay in developers' hearts, and these days is deprecated and instead all DataEngines are turned into plain C++ libraries with specific API. The Plasma weather dataengine from plasma-workspace/dataengines/weather might have already been turned into a library similar to kweathercore, just that the last maintainer (me) was attracted by other even more interesting stuff and no-one had picked up so far. See also https://phabricator.kde.org/T13349 Looking at the current API of KWeatherCore, e.g. KWeatherCore::WeatherForecastSource, I think the Plasma Weather DataEngine and KWeatherCore are very much overlapping, other than the one being a Plasma "library" and the other a normal C++ library. With KWeatherCore now thankfully being created by yours, the weather data related features of the weather dataengine would be ideally merged into KWeatherCore, so that the weather dataengine itself can then be deprecated and all existing weather DataEngine consumers (like kdeplasma-addons/applets/ weather or some on https://store.kde.org/browse/cat/424) could be ported to something based on KWeatherCore (I guess there would be some kind of KWeather qml plugin then for these, next to the KWeatherCore library itself). You will find the normalized data model used by the DataEngine here in the class documentation, which then would ideally be merged into the normalized data model of KWeatherCore, so the existing applets would not need that much porting but get data close to what they are using now: https://invent.kde.org/plasma/plasma-workspace/-/blob/master/dataengines/ weather/ions/ion.h (that normalized data model BTW lacks support for sub-day-granularity for forecasts (like day & night as available with the Canadian service, resulting in bad hack and resulting display in the Plasma applets when using that service), so with room for improvements itself) The weather dataengine also has a plugin mechanism (using DataEngines again for some perhaps random reason, just ignore that) to allow adding support for further weather data provider services by 3rd-party. Such an extension feature would be also nice to have with KWeatherCore. Some more info also here: https://frinring.wordpress.com/2016/04/02/plasma-weather-widget-code-template-available-to-add-your-favorite-weather-data-provider/ Then as Volker already pointed out in good detail, using non-KDE service providers on the internet comes with lots of traps, related to privacy and terms of usage
Re: KDE review for KWeatherCore
Having implemented the weather support for Itinerary, rebasing that onto a more comprehensive framework would indeed be welcome :) I haven't looked too deeply at the implementation or the API yet, most of the feedback below is based on things learned when implementing this for Itinerary. ## api.met.no license and ToS compliance The data is coming from the Norwegian Meteorological Institute, CC-licensed and with an API that doesn't require authentication, well documented and with a mailinglist for service changes and roadmap/disruption announcements. As far as implementing Open Data by organizations/public administration goes this is exemplary and unfortunately quite rare. So at the very least we should follow their license and terms of services as close as possible, in letter and in spirit. Not all of that can be ensured by the library, some of this needs to be handled by the application. In the latter case those requirements need to be clearly documented though. In particular I remember implementing the following aspects: (1) add a point of contact to the User-Agent header, in case your client misbehaves. I only see this done in one place, https://invent.kde.org/ libraries/kweathercore/-/blob/master/src/weatherforecastsource.cpp#L52, which looks suspiciously like a verbatim copy from Itinerary, without even adjusting the contact address. (2) Request throttling. The ToS seem to have been changed in wording since I last looked at this, but the requirement is still similar: Ensure we don't run unnecessary many queries. The Itinerary implementation uses a random interval between 120-150 minutes as lower bound, based on previous suggestions in the ToS. The already suggested daemon is one way to ensure this globally, however I'm very reluctant to suggest a daemon for this, considering the deployment issues this causes for e.g. Flatpak or APKs. Itinerary uses a simple file cache and file mtimes, something like this should also hold up in a multi-process setup with a bit of extra care I think. (3) Attribution, as required by the CC-BY license. This has to happen in the UI, but as a library user I need to know about this requirement, so this needs prominent mention in the docs I'd say. See https://api.met.no/doc/TermsOfService for more details. I also see other services used there I'm not familiar with, are we complying with their licenses and terms of services? ## Privacy considerations (1) Requested geo coordinates are passed to the online API as-is, ie. this is potentially leaking a high-resolution position of the user to the outside. We can't avoid this entirely obviously, but we can reduce the impact by reducing the coordinate resolution. Itinerary uses 1/10th of a degree for this, which unless you get close to the poles are areas of a few kilometers in size, ie. rather than leaking your home address it's only leaking your home town. (2) What does the sunrise API offer beyond the existing sunrise API we have in KF5::Holidays? The latter has the advantage of doing the entire calculation locally. See https://api.kde.org/frameworks/kholidays/html/ namespaceKHolidays_1_1SunRiseSet.html (3) Similarly, we do have a full offline implementation for timezone lookup by geo coordinates here: https://api.kde.org/kdepim/kitinerary/html/ namespaceKItinerary_1_1KnowledgeDb.html#ac409f468badee3c05438695009d7c67f (4) There are http: only requests send to api.geonames.org it seems. Similarly as with the compliance point, some of this can be argued to be the job of the using application. It would seem safer though is the library would try to avoid mistakes to begin with. ## Other observations (1) The license seems to be GPL-2.0-or-later, which is incompatible with the Frameworks license policy. See https://community.kde.org/Policies/ Licensing_Policy (2) The result types (DailyForecast, HourlyForecast, etc) might benefit from being Q_GADGETs and having Q_PROPERTYs added, so they can be directly consumed by QML? (3) binary compatibility measures seem to be missing in a number of places: https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B Regards, Volker On Montag, 21. Dezember 2020 07:16:09 CET hanyoung wrote: > KWeatherCore: https://invent.kde.org/libraries/kweathercore is a library for > querying weather forecast data. During the development of KWeather, we > found the need to have a weather library. KWeatherCore is the result of > extracting weather data fetching code from KWeather. I think having a > dedicated weather library can serve the following propose: - simplify the > KWeather code > - easier to develop a weather daemon > - potentially less code duplication across KDE > > Many of you may have already seen my previous email to kde-devel mailing > list. > Thank you for your constructive suggestions. Here are something I want to clarify: > > I would also propose to consider doing a demon instead, so different > >
KDE review for KWeatherCore
KWeatherCore: https://invent.kde.org/libraries/kweathercore is a library for querying weather forecast data. During the development of KWeather, we found the need to have a weather library. KWeatherCore is the result of extracting weather data fetching code from KWeather. I think having a dedicated weather library can serve the following propose: - simplify the KWeather code - easier to develop a weather daemon - potentially less code duplication across KDE Many of you may have already seen my previous email to kde-devel mailing list. Thank you for your constructive suggestions. Here are something I want to clarify: > I would also propose to consider doing a demon instead, so different > programs/processes all interested in weather forecast data could share the > data The end goal is a daemon indeed, but we want to build the daemon upon the library. This would give us flexibility in the future if we don't want a daemon. At least KWeather and other projects can still benefit from the code. > but we want to make sure we don't end up with two things. I admit there are some overlapped functionalities. But KWeatherCore isn't designed as a weather data provider as Weather DataEngine. Instead, it's trying to be the building block of weather related applications. KWeatherCore saves you the hassle of dealing with APIs, getting locations and converting timezone. You can build a daemon with it, or you can use it in your applications. For example, KItinerary and KWeather use the same weather API, but don't share code. That means two code base to maintain. Regarding the dynamic nature of online APIs, it's better to have one library, so other applications don't need to be worried about their APIs being depraved, and they aren't able to update it in time. Though not currently implemented, KWeatherCore does intend to have weather alerts added. We hope it can be done in this Sok https://community.kde.org/SoK/Ideas/2021#KWeather With this bit added, then the work on weather daemon can be started. Regards, Han Young
