glideapps / quicktype

Generate types and converters from JSON, Schema, and GraphQL
https://app.quicktype.io
Apache License 2.0
11.93k stars 1.05k forks source link

[C++] Generated headers need more helpful comments and should auto-include "Generators.hpp" #2438

Open SimonCahill opened 9 months ago

SimonCahill commented 9 months ago

Given the following method of generating the schemas at Makefile Generation Time:

##################################################################
##  Simple CMake include script which provides functions for    ##
##  generating CSharp and C++ code from JSON schemas.           ##
##                                                              ##
##  Powered by Quicktype.                                       ##
##  Written by Simon Cahill (s.cahill@grimme.de)                ##
##################################################################

# ... snip ...

    execute_process(
        COMMAND ${QUICKTYPE_PATH}
        "-s" "schema"
        "--out" ${OUTPUT_FILE}
        "--namespace" ${NAMESPACE}
        "--lang" "c++"
        "--src" ${JSONS}
        "--alphabetize-properties"
        "--telemetry" "disable"
        "--no-boost"
        "--type-style" "pascal-case"
        "--member-style" "camel-case"
        "--enumerator-style" "upper-underscore-case"
        "--const-style" "west-const"
        "--source-style" "multi-source"
        "--code-format" "with-getter-setter"
        "--include-location" "global-include"
        WORKING_DIRECTORY ${SCHEMAS_DIR}
        OUTPUT_VARIABLE QUICKTYPE_OUTPUT
        RESULT_VARIABLE QUICKTYPE_RESULT
    )
# ... snip ...

I get the following output files in the location I've input as a parameter:

BufferPath.hpp
ChunkCache.hpp
myapp.hpp
Generators.hpp
helper.hpp
ManipTopicMap.hpp
MaxBufferSize.hpp
MessageTypes.hpp
SentChunk.hpp
TotalBufferElements.hpp

Of particular interest are myapp.hpp, helper.hpp, and Generators.hpp.

One quirk of the Quicktype generation system, is you have to include exactly the right file for compilation to work, whereas I'd expect the output to be intelligent enough, so that I can simply include myapp.hpp or helper.hpp, seeming as they're separated by directories, so there isn't much chance something can go wrong in that sense.

When including either of the two aforementioned files, m_chunkCache = json::parse(/*whatever*/) will not work, because Generators.hpp isn't included by any of the other files.
If you don't work with these files all the time, it isn't immediately obvious where the problem is, especially considering the file comment says:

//  To parse this JSON data, first install
//
//      json.hpp  https://github.com/nlohmann/json
//
//  Then include this file, and then do
//
//     ChunkCache.hpp data = nlohmann::json::parse(jsonString);

A hint in the comment to #include <Generators.hpp> would be a first step, but IMO the more sensible thing to do, would be to simply #include "Generators.hpp" in either helper.hpp (which would then really be helpful), or in myapp.hpp.

Both files (myapp.hpp and helper.hpp) include the majority of other headers anyway, so one more wouldn't make much of a difference:

//  To parse this JSON data, first install
//
//      json.hpp  https://github.com/nlohmann/json
//
//  Then include this file, and then do
//
//     myapp.hpp data = nlohmann::json::parse(jsonString);

#pragma once

#include <variant>
#include <nlohmann/json.hpp>
#include "helper.hpp"

#include "BufferPath.hpp"
#include "SentChunk.hpp"
#include "ChunkCache.hpp"
#include "MessageTypes.hpp"
#include "ManipTopicMap.hpp"
#include "MaxBufferSize.hpp"
#include "TotalBufferElements.hpp"
namespace xyz_ipc {
namespace myapp{
}
}

The last point brings me to the other topic.
It seems as though a single template is used for all file comments, which is suboptimal. Especially when:

// helper.hpp data = nlohmann::json::parse(jsonString);
(direct quote)

is written.

Sorry if this issue is a bit rant-y. So far it's been a long day. I may re-format this issue tomorrow after a night's rest.