Changeset View
Standalone View
source/ps/ModIo.h
- This file was added.
/* Copyright (C) 2017 Wildfire Games. | |||||
* This file is part of 0 A.D. | |||||
* | |||||
* 0 A.D. is free software: you can redistribute it and/or modify | |||||
* it under the terms of the GNU General Public License as published by | |||||
* the Free Software Foundation, either version 2 of the License, or | |||||
* (at your option) any later version. | |||||
* | |||||
* 0 A.D. is distributed in the hope that it will be useful, | |||||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | |||||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | |||||
* GNU General Public License for more details. | |||||
* | |||||
* You should have received a copy of the GNU General Public License | |||||
* along with 0 A.D. If not, see <http://www.gnu.org/licenses/>. | |||||
*/ | |||||
#ifndef INCLUDED_MODIO | |||||
#define INCLUDED_MODIO | |||||
#include <string> | |||||
#include "lib/external_libraries/curl.h" | |||||
#include "scriptinterface/ScriptInterface.h" | |||||
class ModIo | |||||
leper: What is this doing in the header? That is an implementation detail only. | |||||
{ | |||||
Done Inline ActionsWhy the newline? leper: Why the newline? | |||||
Not Done Inline ActionsI wanted to separate that from the 0 A.D. headers since it is almost a library include but not quite, but that's just silly. Fixed locally. Itms: I wanted to separate that from the 0 A.D. headers since it is almost a library include but not… | |||||
NONCOPYABLE(ModIo); | |||||
Done Inline ActionsNo newline after this? leper: No newline after this? | |||||
Not Done Inline ActionsFixed the include conventions for all the new files. Itms: Fixed the include conventions for all the new files. | |||||
public: | |||||
ModIo(); | |||||
~ModIo(); | |||||
Done Inline Actions(ModIO?) elexis: (ModIO?) | |||||
const std::vector<std::map<std::string, std::string>>& GetMods(const ScriptInterface& scriptinterface); | |||||
Done Inline ActionsIf I'm not mistaken this data is actually some ModData, since the result from the API is parsed to our own data format. This is not important as long as mod.io is the only way of retrieving it, however that means GetMods maybe should return something that can be used after g_ModIo is deleted (I'd pass a mutable reference to that function). Changing the name would clarify that. Itms: If I'm not mistaken this data is actually some `ModData`, since the result from the API is… | |||||
Not Done Inline ActionsIt is mostly ModData returned by the API (after some parsing). Storing the output from GetMods after g_ModIo stops existing leads you to issues of trying to download something that is not what you want, or might just not work. The data returned by GetMods is only valid for as long as the instance from which you got it exists, and only for that specific instance. So no. leper: It is mostly ModData returned by the API (after some parsing). Storing the output from… | |||||
void DownloadMod(size_t idx); | |||||
private: | |||||
static size_t ReceiveCallback(void* buffer, size_t size, size_t nmemb, void* userp); | |||||
bool ParseGameIdResponse(const ScriptInterface& scriptInterface); | |||||
bool ParseModsResponse(const ScriptInterface& scriptInterface); | |||||
void PrintMods(); | |||||
// Url parts | |||||
std::string m_BaseUrl; | |||||
std::string m_GamesRequest; | |||||
std::string m_GameId; | |||||
// Query parameters | |||||
std::string m_ApiKey; | |||||
std::string m_IdQuery; | |||||
CURL* m_Curl; | |||||
curl_slist* m_Headers; | |||||
char m_ErrorBuffer[CURL_ERROR_SIZE]; | |||||
Not Done Inline ActionsThis looks very over-engineered. leper: This looks very over-engineered. | |||||
Not Done Inline ActionsYeah structs seem to be my little indulgence. Do you see a better-looking alternative? Itms: Yeah `struct`s seem to be my little indulgence. Do you see a better-looking alternative? | |||||
Not Done Inline ActionsThat it uses a struct isn't bad, that it got so big that a struct was a nice solution is what is questionable. std::pair<int, double> seems like a possibility, or use an enum there instead as that already exists. Also while there use enum class. leper: That it uses a `struct` isn't bad, that it got so big that a `struct` was a nice solution is… | |||||
Not Done Inline ActionsI'll repeat the enum class thing since it seem to have been ignored. leper: I'll repeat the `enum class` thing since it seem to have been ignored. | |||||
std::string m_ResponseData; | |||||
std::vector<std::map<std::string, std::string>> m_ModData; | |||||
}; | |||||
extern ModIo* g_ModIo; | |||||
#endif // INCLUDED_MODIO | |||||
Not Done Inline ActionsIf that is a switch statement that is meant to handle everything, then not having a default clause (which NODEFAULT is with some additional code to ensure that at runtime) would cause certain compilers to emit a warning on a switch on an enum given the proper flags. leper: If that is a `switch` statement that is meant to handle everything, then not having a `default`… | |||||
Not Done Inline ActionsYes, so this is correct? Your sentence is not clear to me. Itms: Yes, so this is correct? Your sentence is not clear to me. | |||||
Not Done Inline ActionsSeems like it. leper: Seems like it. | |||||
Done Inline ActionsThis has absolutely not place in a header file, this is an implementation detail only. Leaking the structure of this, both means longer compile times on updates to purely internal structures, and could incite someone to use this outside of the implementation of this class. leper: This has absolutely not place in a header file, this is an implementation detail only. Leaking… | |||||
Done Inline ActionsShould probably be removed, once this has been understood and been implemented. If that does not seem sufficient then it should be rephrased into something that is not a TODO. leper: Should probably be removed, once this has been understood and been implemented. If that does… | |||||
Done Inline ActionsHas this been done? leper: Has this been done? | |||||
Not Done Inline ActionsAs I told you I read this with great interest. Since links stick out in editors I'm sure others have/will. Itms: As I told you I read this with great interest. Since links stick out in editors I'm sure others… | |||||
Not Done Inline ActionsThen I wonder why you answered a question about having downloaded more than the total and still going that way. leper: Then I wonder why you answered a question about having downloaded more than the total and still… | |||||
Done Inline ActionsShould this be a proper doxygen comment? leper: Should this be a proper doxygen comment? | |||||
Done Inline ActionsIf that handles all the cases, not having a default case is preferable since some modern compilers then emit warnings in case one is added, instead of having a runtime check like NODEFAULT. That said this function looks quite useless. leper: If that handles all the cases, not having a `default` case is preferable since some modern… | |||||
Not Done Inline ActionsI added that because of another warning I got, which I fixed differently. Itms: I added that because of another warning I got, which I fixed differently. | |||||
Not Done Inline ActionsWith (partially) manually assigned values for that enum one could do that with a single bit operation even. But that is just me thinking about how to make something shorter or nicer that should most likely not even exist in such a form. leper: With (partially) manually assigned values for that enum one could do that with a single bit… |
What is this doing in the header? That is an implementation detail only.