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

Simplify BOM code #197

Merged
merged 30 commits into from
Aug 22, 2021
Merged

Simplify BOM code #197

merged 30 commits into from
Aug 22, 2021

Conversation

kvid
Copy link
Collaborator

@kvid kvid commented Nov 16, 2020

Currently, this PR does not change any output generated by build_examples.py. Consider implementing the TODO about using 'Description' in BOM heading. However, if this TODO is considered controversial or otherwise might delay merging the PR, then please ignore it for now.

@formatc1702
Copy link
Collaborator

formatc1702 commented Nov 16, 2020

Nice!
Does it make sense to rebase this onto your length_unit2 branch and include any changes necessary for it to work with the new length_unit attribute? (I can't thoroughly test right now, maybe my suggestion does not make sense, and your simplifications eliminate the need for this)

I will merge #196 tonight (squashing the commits, since the overall amount of lines changed is rather small, and adding everyone as a co-author). Then this PR could seamlessly follow afterwards :)

Let me know what you think.

@formatc1702 formatc1702 added this to the v0.3 milestone Nov 16, 2020
@kvid kvid marked this pull request as draft November 16, 2020 22:37
@kvid kvid marked this pull request as draft November 16, 2020 22:37
@kvid kvid marked this pull request as ready for review December 30, 2020 11:29
@kvid kvid mentioned this pull request Mar 21, 2021
@formatc1702
Copy link
Collaborator

I haven't merged this PR since I have seen some more activity on it since you removed the draft status.

If, as per your comment, you think it is ready, I can have a look and integrate it soon :)

@kvid
Copy link
Collaborator Author

kvid commented Mar 21, 2021

I haven't merged this PR since I have seen some more activity on it since you removed the draft status.

That's true. I've added a few commits the last months, but I guess it might be time for a review so other PRs can benefit from these changes soon (if they are accepted).

If, as per your comment, you think it is ready, I can have a look and integrate it soon :)

The currently last commit about "Group common function arguments into a dict" might not look like a simplification, but my goal is to simplify later attribute extensions, like e.g. one of the commits in #219. The same goal is also my motivation behind the preceeding commit in this PR about optional_fields().

src/wireviz/wv_bom.py Outdated Show resolved Hide resolved
kvid added 14 commits April 5, 2021 18:56
- Use the actual BOM as first parameter instead of the whole harness.
- Use a whole AdditionalComponent as second parameter instead of each
  attribute separately.
Move the lambda declaration out of the function scope for common
access from two different functions.
Assign input designators once to a temporary variable for easy reusage.
- Use one common entry loop to consume iterator only once.
- Use same key function for sort() and groupby(),
  except replace None with empty string when sorting.
Seems to be the most popular search alternative:
 https://stackoverflow.com/questions/8653516/python-list-of-dictionaries-search

Raising StopIteration if not found is better than returning None
to detect such an internal error more easily.
Make a list from the group iterator for reusage in sum expressions
and to pick first group entry. The expected group sizes are very small,
so performance loss by creating a temporary list should be neglectable.

Alternativly, itertools.tee(group, 3) could be called to triplicate
the iterator, but it was not chosen for readability reasons.
kvid added 10 commits April 5, 2021 18:56
This type alias describes the possible types of keys and values in
the dict representing a BOM entry.
Build output string in component_table_entry() as the similar strings
in generate_bom(). Repeating a couple of minor if-expressions is small
cost to obtain a more compact and readable main expression.
This way, both BOM and harness.additional_bom_items uses the same
set of keys in their dict entries. This was originally suggested
in a wireviz#115 review, but had too many issues to be implemented then.
This reverts commit 96d393d.
However, raising an exception if failing the BOM index search is still
wanted, so a custom exception is raised instead of returning None.
src/wireviz/wv_bom.py Outdated Show resolved Hide resolved
Co-authored-by: kvid <kvid@users.noreply.github.com>
Copy link
Collaborator

@formatc1702 formatc1702 left a comment

Choose a reason for hiding this comment

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

I've tested the output of this PR and it looks good.
Admittedly, I cannot currently dive deep into every single line of the code, but overall it looks like a good simplification, and I don't want to keep this open much longer than it has to. Anyways, small tweaks and fixes are always possible later.

I will squash-merge to keep the dev branch simple. The detailed history will be preserved in the PR branch for anyone to check.

@formatc1702 formatc1702 merged commit 31d55ff into wireviz:dev Aug 22, 2021
formatc1702 added a commit that referenced this pull request Aug 22, 2021
formatc1702 added a commit that referenced this pull request Aug 23, 2021
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.

2 participants