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

[internal] Create a new module for helper functions of each output format #151

Closed
kvid opened this issue Aug 13, 2020 · 5 comments · Fixed by #192
Closed

[internal] Create a new module for helper functions of each output format #151

kvid opened this issue Aug 13, 2020 · 5 comments · Fixed by #192
Milestone

Comments

@kvid
Copy link
Collaborator

kvid commented Aug 13, 2020

As the number of helper functions for generating HTML is increasing, maybe we should move them into a new wv_html.py file? See also the excerpt from a recent discussion below.

Do you have a set of rules or recommendations about where to insert newlines, i.e. what HTML tags can be on the same row, and when should they be separated?

No hard rules right now, and it's not terribly important. Cells are usually in one row (<td>...</td>). Multiple short opening tags in sequence tend to go together (<tr><td> and the respective closing tags for the outermost table, for example). Row breaks (</tr><tr>) could go in one line but are usually split because they come from iterations of a loop.

Nice. Should we but these words into a comment in the code as soft recommendations?

Maybe into wv_helper.py, or into a new wv_html.py and move the increasing number (at least in my PR #137) of html related helper functions there too?

A new module for the HTML stuff sounds good, as a separate issue/PR!

Have you considered a single space indentation for each sub-level of HTML tag structure?

I thought about it but decided against it; the gains are marginal, the readability of the Python code would suffer; plus it's easy to pass the HTML through some auto-formatter afterwards if one really needs to analyze the structure at some point.
Edit: thinking about it some more, it might not be such a bad idea, I'll try it out and push a commit.

It is not a major issue to me, especially if the Python code gets more complicated and less readable. I will wait and see what you suggest.

It was less painful than anticipated, and I do like the output! Have a look at the latest commit. This will hopefully be the last change before the merge.

Originally posted by @formatc1702 in #136 (comment)

@formatc1702
Copy link
Collaborator

formatc1702 commented Oct 22, 2020

I recently got a similar feeling regarding @Tyler-Ward's BOM generation. (See additional thoughts below)
Before I open a new issue, I suggest widening the scope of this one, to something like "split functions into separate modules" or similar.
Either way, I suggest opening a PR that only moves functions around and only modifies code where absolutely necessary (e.g. passing an object instead of referring to self).
If @kvid or @Tyler-Ward want to take care of this, let me know; otherwise, I can do it myself as well.


Perhaps it would be best to move the BOM functions (currently in Harness.py, as functions inside the Harness class) to a separate wv_bom.py. The functions would then get passed a Harness object instead of self.
Candidates include:

  • get_additional_component_table()
  • get_additional_component_bom()
  • bom()
  • get_bom_index()
  • bom_list()

@formatc1702
Copy link
Collaborator

Regarding the restructuring of HTML-related functions, it is important to distinguish between the HTML used for formatting GraphViz nodes (with all of GraphViz's HTML weirdness), and the actual generation of the .html output file!
Maybe one separate file for each?

@formatc1702
Copy link
Collaborator

Another question that could be discussed here is removing the wv_ prefix from module names.

  • wv_colors.py
  • wv_helper.py
  • wv_html.py
  • wv_bom.py

I am not sure if having a generic name could cause issues if there are other modules with the same name installed on the system (e.g. html.py).

@kvid kvid changed the title [internal] Create a new module for the HTML stuff? [internal] Create a new module for helper functions to each output format? Oct 31, 2020
@kvid
Copy link
Collaborator Author

kvid commented Nov 14, 2020

Another question that could be discussed here is removing the wv_ prefix from module names.

  • wv_colors.py
  • wv_helper.py
  • wv_html.py
  • wv_bom.py

I am not sure if having a generic name could cause issues if there are other modules with the same name installed on the system (e.g. html.py).

I'm not an expert on this issue, and had to google what has been written by others.

  • PEP8 seems not to recommend the prefixes in the current names:

There's also the style of using a short unique prefix to group related names together. This is not used much in Python, but it is mentioned for completeness.

Modules should have short, all-lowercase names. Underscores can be used in the module name if it improves readability.

  • The Python modules with identical names article is relevant, and it seems that absolute imports that became default in Python3 solves most of the problems with module name clashes.

formatc1702 added a commit that referenced this issue Nov 15, 2020
@formatc1702
Copy link
Collaborator

Thanks for the info. I've merged #192 using the wv_ prefix for now, and will probably rename all of the modules closer to release, after doing some tests myself.

@kvid kvid changed the title [internal] Create a new module for helper functions to each output format? [internal] Create a new module for helper functions of each output format Nov 17, 2020
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 a pull request may close this issue.

2 participants