-
Notifications
You must be signed in to change notification settings - Fork 42
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
Support panel's repeat, transparent, span and description by default #145
Conversation
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.
@bazilio91 thanks for your pull request! I'd like to ask for 2 updates to make the code more universal and clean.
|
||
def copy_basic_fields(self, source, dest): | ||
for field in {'description', 'transparent', 'repeat'}: |
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.
There is already support for smth similar in JsonGenerator
through _copy_fields
attribute - it's already defined in e.g. Graph
class. I think it should be pretty simple to define commons fields in PanelsItemBase
and just extend it in all subclasses.
_copy_fields = {'stack', 'fill', 'aliasColors', 'leftYAxisLabel', 'bars', 'lines', 'linewidth', 'y_formats', | ||
'x-axis', 'y-axis', 'points', 'pointradius', 'percentage', 'steppedLine', 'repeat', | ||
'minSpan', 'datasource'} | ||
|
||
def gen_json_from_data(self, data, context): | ||
panel_json = super(Graph, self).gen_json_from_data(data, context) | ||
self.copy_basic_fields(data, panel_json) |
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 instead of defining new method I think it would be better to rely on inheritance and just define method gen_json_from_data
in PanelsItemBase
and handle common properties there.
ok. give me some time, looks like _copy_fields flow needs some kind of refactoring anyway, to make alerts for Graph work. |
one more thing - as you are adding support for new fields please extend tests in https://github.com/jakubplichta/grafana-dashboard-builder/tree/master/tests/grafana_dashboards/components - e.g. https://github.com/jakubplichta/grafana-dashboard-builder/tree/master/tests/grafana_dashboards/components/graph |
@bazilio91 do you think you'll find some time to finish this or should I give it a try? |
#136 related