-
Notifications
You must be signed in to change notification settings - Fork 484
[cleanup] Migrate CodeFormatter to nlohmann::json
#3769
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
base: master
Are you sure you want to change the base?
Conversation
| JSON root{ config_file }; | ||
| if (!root.isOk() || !root.toElement().isArray()) { | ||
| wxString content; | ||
| if (!FileUtils::ReadFileContent(config_file, content)) |
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.
Please run clang-format
| SourceFormatterBase::FromJSON(json); | ||
| m_command = json["command"].toArrayString(); | ||
| m_workingDirectory = json["working_directory"].toString(); | ||
| m_command = JsonUtils::ToArrayString(json["command"]); |
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.
This might throw an exception, lets wrap it with try / catch block.
I suggest that we add new methods in JsonUtils.
For example:
template <typename T>
concept JsonExtractable = requires(const nlohmann::json& j)
{
{j.template get<T>()}->std::same_as<T>;
};
template <typename T>
inline std::optional<T> GetValue(const nlohmann::json& json, const std::string& name) requires JsonExtractable<T>
{
if (!json.contains(name)) {
return std::nullopt;
}
try {
return json[name].get<T>();
} catch (const std::exception&) {
return std::nullopt;
}
}We can later use with default value like this:
m_command = wxString::FromUTF8(JsonUtils::GetValue(json, "command").value_or(""));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.
I noticed afterward that all methods of JsonItem defaults it return value when "empty", whereas nlohmann throws exception for invalid access.
nlohmann::json::value(key, default_value) exists for that case too.
I have indeed to reworks my migrations.
| m_name = json["name"].toString(); | ||
| m_description = json["description"].toString(); | ||
| m_shortDescription = json["short_description"].toString(); | ||
| m_languages = JsonUtils::ToArrayString(json["languages"]); |
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.
See previous comment about exceptions in nlohmann::json
f21ef50 to
ddb2917
Compare
No description provided.