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

feature/easier plugin creation #31

Open
wants to merge 2 commits into
base: beatrice
Choose a base branch
from

Conversation

Dimfred
Copy link

@Dimfred Dimfred commented Jan 23, 2019

code generator for plugin related code

Added a plugin_code_generator.py which generates code plugin specific code. The script is called when the cmake command is executed, and generates files in libraries/utilities/include/graphene/utilities/. The generated files look like this: "generated_*file_name_where_the_generated_code_is_included*_*name_for_the_code_part_where_the_code_is_included*.hpp
Copy link

@pmconrad pmconrad left a comment

Choose a reason for hiding this comment

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

Looks straightforward and simple enough. Can't really comment on the python code itself.

data_dic = dict()
data_dic['plugin_name'] = parsed_data['plugin_name']
data_dic['plugin_namespace'] = parsed_data['plugin_namespace']
data_dic['plugin_project'] = parsed_data['plugin_project']
Copy link

Choose a reason for hiding this comment

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

I'm not a python expert - is it really necessary to copy the fields instead of appending parsed_data directly?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah it would be possible to iterate inside there. But my thought was, we don't have much data hence the speed isn't really necessary here. And like this it's just better separated.

# libraries/app/api.cpp
# libraries/app/application.cpp

generated_files_root_dir = "libraries/utilities/include/graphene/utilities/"
Copy link

Choose a reason for hiding this comment

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

The generated files are not really headers (except generated_api_hpp_include_code.hpp maybe), therefore they shouldn't be put into the include directory. Also don't use .hpp for them, perhaps .hxx or .cxx depending on content.

Copy link
Author

Choose a reason for hiding this comment

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

Will change that for more clearance.

But generally would you say this is way of solving this issue? I think it looks kinda weird to do includes in the middle of the code. But I heard in some projects this is common. Still wouldn't call it good practice.

Choose a reason for hiding this comment

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

Well, I wouldn't call it beautiful. :-)
But if it gets the job done it's ok I guess.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, that's okay for me. :)

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