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

Move BOM management and HTML functions to separate modules #192

Merged
merged 1 commit into from
Nov 15, 2020

Conversation

formatc1702
Copy link
Collaborator

Closes #151.

@formatc1702
Copy link
Collaborator Author

Moving the BOM modules to a separate file might be trickier than expected...

@kvid
Copy link
Collaborator

kvid commented Nov 1, 2020

I know this is WIP, but in case these hints might help you:

  • get_additional_component_table and manufacturer_info_field is not used for BOM and might rather belong to wv_gv_html.py.
  • clean_whitespace is not used for diagram output. It is rather generic, but currently used for BOM.
  • remove_links is rather generic and mainly used for diagram output, but also for .bom.tsv output.

@formatc1702
Copy link
Collaborator Author

formatc1702 commented Nov 1, 2020

Thanks, yes, I'm moving things around as I progress.

Currently, build_examples.py runs without errors, but BOM output is empty.

It seems things are getting lost inside generate_bom() when passing into from the bom_entries variable to bom. I need to figure out why both are needed; I haven't had a deeper look at the latest BOM code.
Update: this was a quick fix.

I think the BOM generation code has grown quite complex, so I need some time to understand it, and make sure we are not passing around more objects than needed, that functions end up in the correct modules, etc.

@formatc1702
Copy link
Collaborator Author

  • get_additional_component_table and manufacturer_info_field is not used for BOM and might rather belong to wv_gv_html.py.
  • get_additional_component_table is tightly linked to get_bom_index() and that one to the actual bom() function, therefore I will choose to leave it in wv_bom.py, at least until we refactor the BOM code, which should be a separate PR.
  • manufacturer_info_field is also used in component_table_entry for generating the mini-BOM within a component node, and it's related to the typical BOM fields pn etc., so I think it also belongs in wv_bom.py. Also, it does not generate any actual HTML.
  • clean_whitespace is not used for diagram output. It is rather generic, but currently used for BOM.
  • remove_links is rather generic and mainly used for diagram output, but also for .bom.tsv output.

I agree they're generic enough to move to wv_helper.py since they are not really tied to BOM or HTML stuff.

@formatc1702 formatc1702 marked this pull request as ready for review November 1, 2020 14:10
@formatc1702
Copy link
Collaborator Author

build_examples.py now runs without errors, and the output is exactly the same as when using the dev branch.

Therefore, unless no glaring errors show up, I would like to merge this soon. Refactoring and optimization of the affected functions can happen in a later PR. Thanks!

@formatc1702 formatc1702 force-pushed the feature/separate-modules branch from cf2db16 to bf2792d Compare November 1, 2020 14:50
@formatc1702 formatc1702 added this to the v0.3 milestone Nov 1, 2020
@formatc1702 formatc1702 force-pushed the feature/separate-modules branch from bf2792d to a96ca7a Compare November 14, 2020 09:04
Copy link
Collaborator

@kvid kvid left a comment

Choose a reason for hiding this comment

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

I can verify that the output is unchanged when generating examples. I do have some simplifications of wv_bom.py in my local repo that I plan to push to a new PR when this is merged, as you requested.

@formatc1702 formatc1702 force-pushed the feature/separate-modules branch from e343fdb to 0e3a24d Compare November 15, 2020 07:41
@formatc1702 formatc1702 merged commit 96bd121 into dev Nov 15, 2020
@formatc1702 formatc1702 deleted the feature/separate-modules branch November 15, 2020 07:42
formatc1702 added a commit that referenced this pull request Nov 15, 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 this pull request may close these issues.

[internal] Create a new module for helper functions of each output format
2 participants