Skip to content
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

Add back logger #49

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add back logger #49

wants to merge 5 commits into from

Conversation

tanaya-mankad
Copy link
Contributor

This branch adds back the Courier logger to be able to pass json reading errors back out to the calling library.
Some other minor changes to the generator code too, mostly formatting and cleanup.
This generator code still doesn't have a test build setup, so the C++ changes aren't tested for functionality yet, but I'd like to hold that to another separate branch.

@spahrenk
Copy link
Contributor

spahrenk commented Jun 18, 2024

It's not clear to me where the loggers should be set. The set_logger functions are not declared in the headers, so the alternative is to just assign them directly, i.e.,

  data_model::ashrae205_ns::logger = hpwh.get_courier();
  data_model::core_ns::logger = hpwh.get_courier();
  data_model::rsintegratedwaterheater_ns::logger = hpwh.get_courier();
  data_model::rstank_ns::logger = hpwh.get_courier();
  data_model::rscondenserwaterheatsource_ns::logger = hpwh.get_courier();
  data_model::rsresistancewaterheatsource_ns::logger = hpwh.get_courier();

Is this the intended use? It seems a bit awkward.
P.S.: If any of them aren't set, it gives a segmentation violation, e.g.,
Process finished with exit code 139 (interrupted by signal 11: SIGSEGV)

P.P.S.: It looks like spelling courierr isused in load-object.h. courier is used everywere else.

@spahrenk
Copy link
Contributor

Would it be possible to automically skip defining empty functions? For example:
void from_json(const nlohmann::json& j, Schema& x) { }

Can we add #include <core.h> to the include list of each of the object header files? It's used for statements like
core_ns::Metadata metadata;

core.h needs to have #include <ashrae205.h>

Copy link
Contributor

@spahrenk spahrenk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some comments based what I'm seeing in the generated code. Some may be addressed in other PRs.

@tanaya-mankad
Copy link
Contributor Author

tanaya-mankad commented Jun 19, 2024

Would it be possible to automically skip defining empty functions? For example: void from_json(const nlohmann::json& j, Schema& x) { }

Can we add #include <core.h> to the include list of each of the object header files? It's used for statements like core_ns::Metadata metadata;

core.h needs to have #include <ashrae205.h>

Yes and yes! Can you write up the first one as a separate Issue? I'll address them in different branches so as not to make this into an omnibus branch. (The core.h I'm already working on in my current branch.)

@tanaya-mankad
Copy link
Contributor Author

It's not clear to me where the loggers should be set. The set_logger functions are not declared in the headers, so the alternative is to just assign them directly, i.e.,

  data_model::ashrae205_ns::logger = hpwh.get_courier();
  data_model::core_ns::logger = hpwh.get_courier();
  data_model::rsintegratedwaterheater_ns::logger = hpwh.get_courier();
  data_model::rstank_ns::logger = hpwh.get_courier();
  data_model::rscondenserwaterheatsource_ns::logger = hpwh.get_courier();
  data_model::rsresistancewaterheatsource_ns::logger = hpwh.get_courier();

Is this the intended use? It seems a bit awkward. P.S.: If any of them aren't set, it gives a segmentation violation, e.g., Process finished with exit code 139 (interrupted by signal 11: SIGSEGV)

P.P.S.: It looks like spelling courierr isused in load-object.h. courier is used everywere else.

Unfortunately, at the moment that is basically the way it's used. The design was to support the factory creation pattern we were using in libtk205. Each "root" class contained a static logger instance, which the code autogenerator could assume would be used in all the support structs that were populated through the root class. We can definitely add a cleaner interface for simple schema.

I'll fix the naming issue for this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants