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

adding --uml-no-inline-groupings-from #872

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pyang/__init__.py
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'
Copy link
Contributor

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.

__date__ = '2022-03-30'
31 changes: 30 additions & 1 deletion pyang/plugins/uml.py
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Contributor

@nkhancock nkhancock Jul 10, 2023

Choose a reason for hiding this comment

The 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?

Copy link
Author

Choose a reason for hiding this comment

The 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?

No, and I agree to the 3 point to switch to --uml-no-inline-groupings-from

Is this intended to only be applicable when the --uml-inline-groupings global option with the --uml-skip-module option is selected?

Yes

If so then the name of this option is to general and is thus misleading and should be improved to make it clearer, e.g., something like --uml-no-inline-groupings-from.

I will adopt to this option

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?

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 --uml-inline-groupings

dest="uml_skip_module",
action="append",
default=[],
metavar="SKIP MODULE",
Copy link
Contributor

Choose a reason for hiding this comment

The 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
metavar="SKIP MODULE",

help="skips given modules, i.e., --uml-skip-module=tailf-ncs"),
Copy link
Contributor

@nkhancock nkhancock Jul 10, 2023

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

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

@nkhancock do you have any common example for this?

Copy link
Contributor

@nkhancock nkhancock Jul 11, 2023

Choose a reason for hiding this comment

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

@nkhancock do you have any common example for this?

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",
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The 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
QQ: Shall I pull out the changes of --uml-add-legend and open a new PR when it's ready?

Copy link
Contributor

@nkhancock nkhancock Jul 11, 2023

Choose a reason for hiding this comment

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

Shall I pull out the changes of --uml-add-legend and open a new PR when it's ready?

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
Expand Down Expand Up @@ -166,6 +176,7 @@ class uml_emitter:
_ctx = None
post_strings = []
module_prefixes = []
ctx_grp_files = set()

def __init__(self, ctx):
self._ctx = ctx
Expand All @@ -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
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

The 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')
Copy link
Contributor

@nkhancock nkhancock Jul 10, 2023

Choose a reason for hiding this comment

The 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:
Expand Down