-
Notifications
You must be signed in to change notification settings - Fork 348
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
adding --uml-no-inline-groupings-from #872
base: master
Are you sure you want to change the base?
Changes from 1 commit
8dc80f7
a58f297
ae249d6
23f5bc4
0d27a44
d98c82b
1e73120
1e5e94d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
"""The pyang library for parsing, validating, and converting YANG modules""" | ||
|
||
__version__ = '2.5.3' | ||
__version__ = '2.5.4' | ||
__date__ = '2022-03-30' |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -93,6 +93,16 @@ def add_opts(self, optparser): | |||
optparse.make_option("--uml-filter-file", | ||||
dest="uml_filter_file", | ||||
help="NOT IMPLEMENTED: Only paths in the filter file will be included in the diagram"), | ||||
optparse.make_option("--uml-skip-module", | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The --uml-inline-groupings option is then not applied to those uses statements for groupings in the list of modules specified in --uml-skip-module? Correct? Is this intended to only be applicable when the --uml-inline-groupings global option with the --uml-skip-module option is selected? If so then the name of this option is too general and is thus misleading and should be improved to make it clearer, e.g., something like --uml-no-inline-groupings-from. Also if --uml-inline-groupings and --uml-skip-module are not specified wouldn't it be expected that if a legend is specified, then the modules of the groupings should be listed as when --uml-inline-groupings=true and --uml-skip-module is not empty? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No, and I agree to the 3 point to switch to
Yes
I will adopt to this option
You point is true, but if I don't use --uml-inline-groupings the uml is not traversing to it's groupings (other modules) so far. Now I see based on your comment the legend can be improved to touch the submodule too to keep in generic not just for |
||||
dest="uml_skip_module", | ||||
action="append", | ||||
default=[], | ||||
metavar="SKIP MODULE", | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just use the default metavar, i.e., UML_SKIP_MODULE to indicate the context of the UML plugin; or at least SKIP_MODULE.
Suggested change
|
||||
help="skips given modules, i.e., --uml-skip-module=tailf-ncs"), | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Text should be improved to make it clearer what the actual skipping of modules means. See my comment on line 96. Also avoid referencing specific modules that may not be available to everyone, e.g., tailf-ncs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nkhancock do you have any common example for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
A possible candidate could be ietf-yang-push. This has internal groupings and also uses an external grouping from ietf-yang-patch. |
||||
optparse.make_option("--uml-add-legend", | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name of this option implies that the user could define or control the rendering of a legend in the diagram, whereas this legend is specific to the --uml-skip-module option. It also needs to be considered how such an option could co-exist with a possible new option to render a user-specified legend going forward. Maybe consider using a note instead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Initially I tried note, which is mapping to the last class (It needed more logic to map to right class etc) let me revisit and add the changes. @nkhancock There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's up to you. If the changes are completely independent, maybe separate PRs would make it easier to review. It's basically up to you. |
||||
dest="uml_add_legend", | ||||
action="store_true", | ||||
help="Adds legend about grouping yang file in the UML"), | ||||
kirankotari marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
] | ||||
if hasattr(optparser, 'uml_opts'): | ||||
g = optparser.uml_opts | ||||
|
@@ -166,6 +176,7 @@ class uml_emitter: | |||
_ctx = None | ||||
post_strings = [] | ||||
module_prefixes = [] | ||||
ctx_grp_files = set() | ||||
|
||||
def __init__(self, ctx): | ||||
self._ctx = ctx | ||||
|
@@ -188,6 +199,8 @@ def __init__(self, ctx): | |||
self.ctx_title = ctx.opts.uml_title | ||||
|
||||
self.ctx_inline_augments = ctx.opts.uml_inline_augments | ||||
self.ctx_skip_module = set(ctx.opts.uml_skip_module) | ||||
self.ctx_add_legend = ctx.opts.uml_add_legend | ||||
|
||||
no = ctx.opts.uml_no.split(",") | ||||
self.ctx_leafrefs = not "leafref" in no | ||||
|
@@ -255,6 +268,10 @@ def emit(self, modules, fd): | |||
|
||||
if not self.ctx_filterfile: | ||||
self.post_process_module(module, fd) | ||||
|
||||
if self.ctx_add_legend: | ||||
self.emit_uml_legend(module, fd) | ||||
|
||||
if not self.ctx_filterfile: | ||||
self.post_process_diagram(fd) | ||||
|
||||
|
@@ -413,7 +430,14 @@ def emit_child_stmt(self, parent, node, fd, cont = True): | |||
if hasattr(node, 'i_grouping') and (self._ctx.opts.uml_inline) and cont: | ||||
grouping_node = node.i_grouping | ||||
if grouping_node is not None: | ||||
# skip grouping modules if given | ||||
module = str(grouping_node.main_module()).split()[-1] | ||||
if module in self.ctx_skip_module: | ||||
fd.write('%s : %s {uses} \n' %(self.full_path(parent), node.arg)) | ||||
return | ||||
# inline grouping here | ||||
# collecting grouping module file names | ||||
self.ctx_grp_files.add(str(grouping_node.pos).split(':')[0].split('/')[-1]) | ||||
kirankotari marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
# sys.stderr.write('Found target grouping to inline %s %s \n' %(grouping_node.keyword, grouping_node.arg)) | ||||
for children in grouping_node.substmts: | ||||
# make the inlined parent to parent rather then the grouping to make full path unique | ||||
|
@@ -591,7 +615,12 @@ def emit_module_header(self, module, fd): | |||
def emit_module_class(self, module, fd): | ||||
fd.write('class \"%s\" as %s << (M, #33CCFF) module>> \n' %(self.full_display_path(module), self.full_path(module))) | ||||
|
||||
|
||||
def emit_uml_legend(self, module, fd): | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name of this function is too general for the function it performs. It is specific to the --uml-skip-modules option. To allow possible future support for a user to define a legend with a given text, this function should just accept a text to be shown within the legend box and a new function specific to --uml-skip-modules, e.g., emit_uml_skip_modules_legend could be defined that creates the required text for the skip module legend and calls the general function. |
||||
fd.write('legend \n') | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should a legend be written if there are no files/modules to list, i.e., only when self.ctx_grp_files is not empty? If I don't specify either --uml-inline-groupings or --uml-skip-module, but specify --uml-add-legend, then a legend is displayed without any files listed. Also if --uml-inline-groupings and --uml-skip-module are not specified wouldn't it be expected that if a legend is specified, then the modules of the groupings should be listed as when --uml-inline-groupings=true and --uml-skip-module is not empty? |
||||
fd.write('Grouping data pulled from \n') | ||||
kirankotari marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
for file in self.ctx_grp_files: | ||||
fd.write(f' {file} \n') | ||||
fd.write('endlegend \n') | ||||
|
||||
def emit_uml_footer(self, module, fd): | ||||
if self._ctx.opts.uml_footer is not None: | ||||
|
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 suggest that this decision is left to the repository owners.