Feature/additional components

[Tyler-Ward]

Draft implementation of #50.

An exmple file to work with this is shown below.

connectors:
  X1:
    type: Powerpole
    pinout: [GND, VCC]
    ignore_in_bom: true
    additional_components:
      -
        type: housing
        qty: 1
        unit: ea
        manufacturer: Anderson
        manufacturer_part_number: 1327G6
        internal_part_number: powerpole-black
      -
        type: housing
        qty: 1
        unit: ea
        manufacturer: Anderson
        manufacturer_part_number: 1327
        internal_part_number: powerpole-red
      -
        type: crimp
        qty: pincount
        unit: ea
        manufacturer: Anderson
        manufacturer_part_number: 1331
        internal_part_number: powerpole-crimp-30A
  X2: &mf3-5
    type: Molex Micro-Fit
    subtype: female
    pinout: [GND, VCC, A, B, C]
    manufacturer_part_number: 436450500
    manufacturer: Molex
    internal_part_number: MF3.0-5
    additional_components:
      -
        type: crimp
        qty: connectioncount
        unit: ea
        manufacturer: Molex
        manufacturer_part_number: 43030-0007
        internal_part_number: MF3.0-Crimp

  X3:
    <<: *mf3-5

cables:
  W1: &wire_power # define template
    colors: [BK, RD] # number of wires implicit in color list
    gauge: 0.25 # assume mm2 if no gauge unit is specified
    show_equiv: true
    length: 0.2
    manufacturer_part_number: 123456

  W2:
    <<: *wire_power

connections:
  -
    - X1: [1-2]
    - W1: [1-2]
    - X2: [1-2]
  -
    - X1: [1-2]
    - W2: [1-2]
    - X3: [1-2]

[Tyler-Ward]

This is now just about functional but needs cleanup of the code, making the output look better, error checking, altered names to match #114 and addition of later sugestions in #50. Will continue this after #114 is done.

[formatc1702]

Not too shabby!

My feeling is that the mini-BOMs within the nodes should be either one line per item (might stretch the nodes, I know),
one multiline cell per item (to easily distinguish separate items visually) or something similar. If I have a new suggestion, I can let you know.

Note: The unit is missing on the cable's sleeving, as well as the crimps and housing (should be showing ea)

[formatc1702]

Another idea which might keep the diagram simple, but definitely require the BOM table, is the following:

• Auto-assign an Item Number (e.g. 1, 2, 3, ... or 10, 20, 30, ...) to every BOM item, show item number as first column in master BOM
• In the mini-BOMs, show one line = one cell per item in the format:
  {qty} {unit} x {itemno} for maximum minimalism, forcing some cross-referencing
  or
  {qty} {unit} x {itemno} ({description}) to keep it user-friendly but at least keeping the P/Ns out of it.

One could do a separate cell for each {} but hide the vertical borders within a line to get a nice table look without to much visual clutter?

I get the feeling that someone going as far as specifying every single crimp and mm of heatshrink, is also the kind of person that would want a proper BOM and not be satisfied with a PNG image anyway 🤓 so it wouldn't matter if the PNG was missing every bit of detail.

[formatc1702]

Quick reminder to use imperative mood in git commit messages ;)

[Tyler-Ward]

This branch has now been brought up to date with dev and the rest of the ideas from #50 have been aded in.

Merging the Bom items to a single cell definatley looks better. Using bom indexing to make the graph simpler sounds like a good idea, this will need some refactoring of the BOM generation code to get this to work but should be fairly easy to add once that is done. Other than that I think the only other thing that needs adding is adding a tutorial and syntax description (once #111 is merged in).

[Tyler-Ward]

The Bom has been refactored to remove duplications and add an id that can be looekd up when generating a graph. this is then used for the aditional components sections. The aditional component mode can be changed by a harness attribute to slect the option above but this will need #158 or similar in order to be used

[kvid]

@Tyler-Ward Thank you for this very useful contribution. I'm just another contributor, like you. @formatc1702 is the owner that approves and merges PRs into dev when he thinks they are ready. He has been on vacation for several weeks, and I decided to review your PR and suggest some changes that I believe will help you getting this PR accepted, but remember that @formatc1702 might not share all my opinions.

• I like the BOM deduplication at the end as it enables removing a lot of other complex deduplication code.
• Consider moving more duplicated code into common (generic if needed) functions.

[kvid]

Initially, I was going to suggest forcing extra['unit'] = 'm' in this case, but then I discovered a use case where that would be a bad idea: When specifying, e.g. 5 cable clamps per meter, then the unit cannot be meter. Therefore, I conclude: Keep it as is to also enable such use cases.

[kvid]

I suggest using f-string like the rest of the code, call it a multiplier, and add quotes around the value:

    raise ValueError(f'invalid qty multiplier "{extra["qty_multiplier"]}"')

[kvid]

I suggest removing all 14 None arguments here. The second parameter of .get() and the last 4 parameters of component_table_entry() are all optional with default value=None. Simpler code is normally easier to read.

[kvid]

I suggest removing all 14 None arguments here. The second parameter of .get() and the last 4 parameters of component_table_entry() are all optional with default value=None. Simpler code is normally easier to read.

[kvid]

I suggest using f-string like the rest of the code, call it a multiplier, and add quotes around the value:

    raise ValueError(f'invalid qty multiplier "{extra["qty_multiplier"]}"')

[kvid]

I suggest using f-string like the rest of the code, call it a multiplier, and add quotes around the value:

    raise ValueError(f'invalid qty multiplier "{extra["qty_multiplier"]}"')

[kvid]

Consider instead: if item.get('designators'):

[kvid]

Consider joining these two lines by removing the assignment and return the expression directly.

[kvid]

Consider joining these two lines by removing the assignment and return the expression directly.

[kvid]

I suggest removing all 7 None arguments in the 4 lines above. The second parameter of .get() is optional with default value=None. Simpler code is normally easier to read.

[Tyler-Ward]

Thanks for your detailed comments @kvid. They all look good so have integrated them. Moving the components to a dataclass rather than a dict definatly helped the readability of the code in harness.py

[kvid]

• Thank you for accepting my suggestions.
• See code comments for some extra simplification suggestions.

[kvid]

Consider adding a @property decorator in a new line in front of this method to access the value as a property, e.g. extra.long_name (without the parentheses).

[kvid]

Consider instead if connector.additional_components:

[kvid]

Consider joining these two lines into one:

qty = extra.qty * calculate_qty_multiplier_connector(extra.qty_multiplier, connector)

[kvid]

Consider moving this function into the Cable class as a method:

    def calculate_qty_multiplier(self, qty_multiplier: Optional[CableMultiplier]) -> float:

[kvid]

Consider using elif like below for consistence.

[kvid]

Consider appending a comment to this line:

# pincount | populated (for connectors), or wirecount | terminations | length | total_length (for cables)

Edit: However, an even better approach is to add the same information using type aliases that also can be used in other type annotations:

# from typing import Literal (New in Python 3.8)

ConnectorMultiplier = str # = Literal['pincount', 'populated']
CableMultiplier = str # = Literal['wirecount', 'terminations', 'length', 'total_length']

@dataclass
class AdditionalComponent:
    ...
    qty_multiplier: Union[ConnectorMultiplier, CableMultiplier, None] = None
    ...
...
    def calculate_qty_multiplier(self, qty_multiplier: Optional[ConnectorMultiplier]) -> int:

Note that the Literal type specifier is in comments to avoid requiring Python 3.8.

[Tyler-Ward]

Thanks for the additional suggestions, Have added them as well, I left the swapping from dict to dataclass code the same as it was for now as the one liner is a bit harder to read but happy to swap to that if other people think that is better.

[kvid]

Thank you again for accepting most of my suggestions. I also like that you argue against suggestions that might harm readability. I'm sorry for suggesting more changes so many times, but I often find it hard to see all improvement issues at once. Some issues become much easier too see after some other improvement steps have been done first. I've marked some comments with DRY for Don't Repeat Yourself because I prefer to avoid repeating the same code twice when it's possible to rewrite it to a loop, function call, or some other way to avoid code duplication without harming readability.

[kvid]

Consider joining the two lines into, e.g.

return self.type.capitalize() + (f', {self.subtype}' if self.subtype else '')

or slightly shorter, but maybe less readable:

return ', '.join(filter(None, [self.type.capitalize(), self.subtype]))

[kvid]

DRY: Consider moving these 9 lines above (plus suggested changes) into a new method and just call it from here:

Suggested change

	if connector.additional_components:
	rows.append(["Additional components"])
	for extra in connector.additional_components:
	qty = extra.qty * connector.get_qty_multiplier(extra.qty_multiplier)
	if(self.mini_bom_mode):
	id = self.get_bom_index(extra.description, extra.unit, extra.manufacturer, extra.mpn, extra.pn)
	rows.append(component_table_entry(f'{id} ({extra.type.capitalize()})', qty, extra.unit))
	else:
	rows.append(component_table_entry(extra.description, qty, extra.unit, extra.pn, extra.manufacturer, extra.mpn))
	rows.extend(self.additional_components_table_entries(connector))

[kvid]

Personally, I would prefer some prefix to the BOM id number in the component table for increased readability,
e.g. '#', 'No ' or 'BOM Id ', but other users might disagree (e.g. maybe also different preferences in US and UK), and I suggest supporting all variations by accepting a user specified prefix as an optional string value in the mini_bom_mode parameter in addition to the current True and False values - similar to the cable.shield parameter values. I therefore suggest inserting this code just below the current assignment of the id variable:

if isinstance(self.mini_bom_mode, str):
    id = f'{self.mini_bom_mode}{id}'

The if self.mini_bom_mode: above will still work as intended for both bool and str values because a non-empty string prefix also evaluates to True.

[Tyler-Ward]

I tested with the # and it looks much better than the number on its own, Im not sure if there is a huge amount to be gained by allowing the user to choose, given this would be the only place to have this customisation (unless i am missing anywhere). It might make sense to leave it out the customisation for now and add it later if it is uefull.

[kvid]

I suggest renaming items to group_entries and item to inp to avoid any confusion with the BOM item column name.

[kvid]

As above, I suggest using entry as BOM entry variable.

[kvid]

As above, I suggest using entry as BOM entry variable.

[kvid]

As above, I suggest using entry as BOM entry variable.

[kvid]

Please remove the outer parentheses from the if in this loop even if not accepting the whole suggestion below.

Edit: The rest of this suggestion doesn't hurt, but is probably not really needed if you ignore my other suggestion with all the issues.

Suggested change

	for bom_item in self.bom():
	if((bom_item['item'], bom_item['unit'], bom_item['manufacturer'], bom_item['mpn'], bom_item['pn']) == (item, unit, manufacturer, mpn, pn)):
	return bom_item['id']
	for entry in self.bom():
	if tuple(entry.get(key) for key in ('item', 'unit', 'manufacturer', 'mpn', 'pn')) == (item, unit, manufacturer, mpn, pn):
	return entry['id']

Using entry.get() to handle missing keys is needed if accepting suggestion using dict comprehension from additional_bom_items.

[kvid]

I assume it is a bug that this code line does not end with if cable.show_name else None as for connectors, but by using a common method, the bug should be fixed. Unless my assumption is wrong, this bug is a good example on the advantages with the DRY principle.

[Tyler-Ward]

Cables didn't previously use the show_name field when generating the bom, however it is used by cables in the graph so makes sense to add to the bom code as well.

[Tyler-Ward]

No Problem, thanks for taking the time to keep reviewing. Some more good sugestions here, have implemented most of them and added comments on the others

[Tyler-Ward]

I mostly program in c/c++ so if statement muscle memory that is likey the cause of that, now removed.

[Tyler-Ward]

Not quite sure how i made that typo but that should have been fields, now fixed

[Tyler-Ward]

I tested with the # and it looks much better than the number on its own, Im not sure if there is a huge amount to be gained by allowing the user to choose, given this would be the only place to have this customisation (unless i am missing anywhere). It might make sense to leave it out the customisation for now and add it later if it is uefull.

[Tyler-Ward]

Cables didn't previously use the show_name field when generating the bom, however it is used by cables in the graph so makes sense to add to the bom code as well.

[kvid]

Thank you once again for accepting most of my numerous suggestions. Please let me know when you are tired of improving details in this PR, and prefer raising a new issue to handle further improvements after merge-in, but please take a look at least at the two suggestions beginning with lines 441 and 448 of Harness.py.

Edit: I'm really glad you've managed to both add this useful feature with additional components and that you in the process also simplified the BOM generating code that will be a great benefit to other PRs. The owner wrote in #101 (comment) that he will finish the remaining PRs of v0.2 first, to avoid "feature creep", and then process the other PRs for v0.3. I suggested to him that your PR is a good candidate to start with, and hope he agrees when he arrives that stage. That means you should be prepared to rebase your PR branch on top of the new dev right after the v0.2 release. Thank you for all the good work!

Edit2: One of my code comments in this review is still tagged as Pending which I normally only see on comments before submitting the review, and I wonder if this comment was i fact submitted together with the rest of the review. Can you check if you see my comment as part of this review on line 343 of Harness.py with this contents:

• It seems you forgot to remove these parentheses as mentioned in your Feature/additional components #115 (comment).

[kvid]

It seems you forgot to remove these parentheses as mentioned in your #115 (comment).

[kvid]

Suggested change

	bom_entries.append(
	{
	'item': part.description,
	'qty': qty,
	'unit': part.unit,
	'manufacturer': part.manufacturer,
	'mpn': part.mpn,
	'pn': part.pn,
	'designators': component.name if component.show_name else None
	}
	)
	bom_entries.append({
	'item': part.description,
	'qty': qty,
	'unit': part.unit,
	'manufacturer': part.manufacturer,
	'mpn': part.mpn,
	'pn': part.pn,
	'designators': component.name if component.show_name else None
	})

Why not use the same compact style as you use in the bom() method below?

[kvid]

Please remove the outer parentheses from the if in this loop even if not accepting the whole suggestion below.

Edit: The rest of this suggestion doesn't hurt, but is probably not really needed if you ignore my other suggestion with all the issues.

Suggested change

	for bom_item in self.bom():
	if((bom_item['item'], bom_item['unit'], bom_item['manufacturer'], bom_item['mpn'], bom_item['pn']) == (item, unit, manufacturer, mpn, pn)):
	return bom_item['id']
	for entry in self.bom():
	if tuple(entry.get(key) for key in ('item', 'unit', 'manufacturer', 'mpn', 'pn')) == (item, unit, manufacturer, mpn, pn):
	return entry['id']

Using entry.get() to handle missing keys is needed if accepting suggestion using dict comprehension from additional_bom_items.

[kvid]

Suggested change

	self._bom.append({
	'item': shared['item'], 'qty': round(total_qty, 3), 'unit': shared['unit'], 'designators': designators,
	'manufacturer': shared['manufacturer'], 'mpn': shared['mpn'], 'pn': shared['pn']
	})
	self._bom.append({**group_entries[0], 'qty': round(total_qty, 3), 'designators': designators})

[kvid]

Suggested change

shared = group_entries[0]

If accepting my other suggestion that removes all usage of this variable, then it's no longer needed.

[kvid]

Suggested change

	total_qty = sum(i['qty'] for i in group_entries)
	total_qty = sum(e['qty'] for e in group_entries)

I suggest using e or entry as loop variable here.

[kvid]

Edit: This suggestion have too many issues and is not worth the effort at this stage I now believe after thinking a bit more. I'm really sorry for the extra noise. You can safely ignore this.

The whole loop above can be replaced with one of the alternative one-liners below, but there are some issues that might stop you from accepting them:

bom_entries.extend([{**entry, 'item': entry.get('description', '')} for entry in self.additional_bom_items])

or

bom_entries.extend([{k.replace('description', 'item'): v for k, v in entry.items()} for entry in self.additional_bom_items])

If we in the code rename the 'item' key to 'description' in all BOM entry dicts (the column header can still be kept as 'Item' if needed), then it would be even simpler and more consistent, but I'm not sure if such a change might be out of scope for this PR:

bom_entries.extend(self.additional_bom_items)

Three are some minor issues with each of these three alternative one-liners:

1. The dict entries produced by the first alternative have an extra 'description' value that is never used. It's not hard to remove it, but is it worth complicating the code for that as long as the extra dict value in memory does not affect any output?
2. The item/description values produced by the second and third alternatives might be absent if not present in the input YAML. If that might be a problem, it can be fixed in the deduplication part below.
3. All alternatives might produce entries where some of the keys are absent if not present in the input YAML. See the two suggestions about using get() in bom_types_group and get_bom_index() to handle absent keys.

[kvid]

Suggested change

	index = 1
	for item in self._bom:
	item["id"] = index
	index += 1
	self._bom = [{**entry, 'id': index} for index, entry in enumerate(self._bom, 1)]

[kvid]

Edit: This suggestion doesn't hurt, but is not really needed if you ignore my other suggestion with all the issues.

Suggested change

	bom_types_group = lambda bt: (bt['item'], bt['unit'], bt['manufacturer'], bt['mpn'], bt['pn'])
	bom_types_group = lambda bt: tuple(bt.get(key) for key in ('item', 'unit', 'manufacturer', 'mpn', 'pn'))

Using bt.get() to handle missing keys is needed if accepting suggestion using dict comprehension from additional_bom_items.

[formatc1702]

Progress on this PR looks fantastic, thank you guys for your heavy work so far.
@Tyler-Ward, I hope you're able to include @kvid's latest suggestions soon!

Fair warning: I might have 1-2 little nit-picks as well before I merge, but @kvid has done the heavy lifting and overall, this looks very good!

[Tyler-Ward]

I have added the majority of the suggestions above, excluding the one that was sigested to skip and the one commented on below which has highlighted some bugs that need fixing which I will sort tomorrow.

Thank you once again for accepting most of my numerous suggestions. Please let me know when you are tired of improving details in this PR, and prefer raising a new issue to handle further improvements after merge-in, but please take a look at least at the two suggestions beginning with lines 441 and 448 of Harness.py.

Thanks for the additional suggestions, Happy to keep on making edits now if there are further things to improve. or for them to be split into seperate tickets.

Edit2: One of my code comments in this review is still tagged as Pending which I normally only see on comments before submitting the review, and I wonder if this comment was i fact submitted together with the rest of the review. Can you check if you see my comment as part of this review on line 343 of Harness.py with this contents:

That appears for me and has been fixed (along with a couple of others)

[kvid]

Thank you for accepting most of my suggestions again. Se my suggested solution to the problem you describe. I haven't had time to look at the other modifications you made yet.

[kvid]

Suggested change

	if(entry['item'], entry['unit'], entry['manufacturer'], entry['mpn'], entry['pn']) == (item, unit, manufacturer, mpn, pn):
	if (entry['item'], entry['unit'], entry['manufacturer'], entry['mpn'], entry['pn']) == tuple(clean_whitespace(v) for v in (item, unit, manufacturer, mpn, pn)):

[kvid]

If my other suggestion solves your problem, then this can be removed:

Suggested change

item_list = [remove_line_breaks(subitem) for subitem in item_list] # remove line breaks if present

[Tyler-Ward]

That would catch some of the issues, the one I found that is easiest to end up in however is the following where a space is added before a comma. we could add code to replace any space comma combinations with a single comma, not sure if that would cause any other issues but can investigate that later today.

Below is a section of a bom where the connector type was split into two lines

Id	Item	Qty	Unit	Designators	P/N	Manufacturer	MPN
3	Connector, Molex Micro-Fit , female, 5 pins	2		X2, X3	MF3.0-5	Molex	436450500
4	Connector, Molex Micro-Fit, female, 5 pins	1		ZZ	MF3.0-5	Molex	436450500

[kvid]

That would catch some of the issues, the one I found that is easiest to end up in however is the following where a space is added before a comma. we could add code to replace any space comma combinations with a single comma, not sure if that would cause any other issues but can investigate that later today.

Please share the YAML input that provoke the undesired output.

Below is a section of a bom where the connector type was split into two lines

Id Item Qty Unit Designators P/N Manufacturer MPN
3 Connector, Molex Micro-Fit , female, 5 pins 2   X2, X3 MF3.0-5 Molex 436450500
4 Connector, Molex Micro-Fit, female, 5 pins 1   ZZ MF3.0-5 Molex 436450500

I assume the first of these two entries was produced by specifying additional_components for X2 and X3. Am I right? In that case, the problem is caused by the comma added in AdditionalComponent.description(). I suggest calling clean_whitespace() on the attribute that is concatenated to the comma separator without a space between, i.e.:

    @property
    def description(self) -> str:
        return clean_whitespace(self.type.capitalize()) + (f', {self.subtype}' if self.subtype else '')

I could not find any other code that concatenated a string attribute with a non-space separator before inserting into Harness.bom_entries.

Edit: This means all newlines in AdditionalComponent.type will be ignored also in the diagram. I that is undesired, then we probably only should remove any trailing whitespace before the comma separator in this property function.

[Tyler-Ward]

The yaml that generated that bom is included at the end of the post (probably not the best test case in its current form but can be turned into one).

The error is with the bom line for the connector itself (X2 and X3) where the type was split into two lines. ZZ is a aditional bom item that matches the output of a connector without a linebreak in its type. The extra space is added before the comma because the trailing newline is kept when the description string is built from the component variables (line 374 in the current state).

connectors:
  X1:
    type: Powerpole
    pinlabels: [GND, VCC]
    ignore_in_bom: true
    additional_components:
      -
        type: housing
        subtype: Anderson Powerpole 15/45, Black
        qty: 1
        manufacturer: Anderson
        mpn: 1327G6
        pn: powerpole-black
      -
        type: housing
        subtype: Anderson Powerpole 15/45, Red
        qty: 1
        manufacturer: Anderson
        mpn: 1327
        pn: powerpole-red
      -
        type: crimp
        subtype: Anderson Powerpole 15/45, 30A
        qty_multiplier: pincount
        manufacturer: Anderson
        mpn: 1331
        pn: powerpole-crimp-30A
    notes: "notes go here"
  X2: &mf3-5
    type: |
      Molex
      Micro-Fit
    subtype: female
    pinlabels: [GND, VCC, A, B, C]
    mpn: 436450500
    manufacturer: Molex
    pn: MF3.0-5
    additional_components:
      -
        type: crimp
        qty_multiplier: populated
        manufacturer: Molex
        mpn: 43030-0007
        pn: MF3.0-Crimp

  X3:
    <<: *mf3-5

  F1:
    style: simple
    autogenerate: true
    type: Crimp ferrule
    subtype: 0.5 mm²
    color: OG # optional color
    additional_components:
      -
        type: crimp housing

cables:
  W1: &wire_power # define template
    colors: [BK, RD] # number of wires implicit in color list
    gauge: 0.25 # assume mm2 if no gauge unit is specified
    show_equiv: true
    length: 0.2
    mpn: 123456

  W2:
    <<: *wire_power
    additional_components:
      -
        type: |
            Sleve
        subtype: |
            Braided nylon,
            black, 3mm
        qty_multiplier: length
        unit: m
        pn: SLV-1

  W3:
    <<: *wire_power

connections:
  -
    - X1: [1-2]
    - W1: [1-2]
    - X2: [1-2]
  -
    - X1: [1-2]
    - W2: [1-2]
    - X3: [1-2]
  -
    - X1: [1-2]
    - W3: [1-2]
    - F1

additional_bom_items:
  - # define an additional item to add to the bill of materials (does not appear in graph)
    description: |
        Connector,
        Molex Micro-Fit, female, 5 pins
    qty: 1
    designators:
      - ZZ
    manufacturer: Molex
    mpn: 436450500
    pn: MF3.0-5

[kvid]

The yaml that generated that bom is included at the end of the post (probably not the best test case in its current form but can be turned into one).

Thank's for sharing. I believe it's a good candidate for future regression tests - you can suggest an improved version when you have the time.

The error is with the bom line for the connector itself (X2 and X3) where the type was split into two lines. ZZ is a aditional bom item that matches the output of a connector without a linebreak in its type. The extra space is added before the comma because the trailing newline is kept when the description string is built from the component variables (line 374 in the current state).

You are right. That means we need the same here: Calling clean_whitespace() separately on all the attribute that might be concatenated to the comma separator without a space between, i.e. connector.type, connector.subtype, and cable.type. (More or less your alternative 3.)

A better alternative is then to expand clean_whitespace() (as you suggested in #115 (comment)) instead of these separate calls (including the separate call in the property method I suggested earlier today):

def clean_whitespace(inp):
    return ' '.join(inp.split()).replace(' ,', ',') if isinstance(inp, str) else inp

In any case, my other suggestions should still be valid (except the separate call in AdditionalComponent.description() if you accept expanding clean_whitespace()).

I assume comma is the only non-space separator that can cause such problems...

[Tyler-Ward]

I have looked through the fields that support newlines and in the bom they are all folowed by a comma or a space so that whitespace cleanup function will work and has been added.

I have rebased ontop of the latest dev to fix the merge conflict and also added the new syntax additions to the documentation.

[kvid]

Thank you for accepting some of my suggestions again, and for rebasing on top of the new dev.

[kvid]

I suggest reversing your latest changes here to remove the two .strip() calls:

Suggested change

	return self.type.capitalize().strip() + (f', {self.subtype.strip()}' if self.subtype else '')
	return self.type.capitalize() + (f', {self.subtype}' if self.subtype else '')

• I assume this was one of the things you tried to avoid space before comma in the BOM.
• Now, when you remove those spaces in clean_whitespace(), no such actions should be needed here.
• The only reason for the user to include newlines, is to adapt the text in the diagram, and then we shouldn't remove it in the diagram without a very good reason. It's better to let the user see the consequence if it was a mistake, and then he can remove it from the input.

[Tyler-Ward]

This is needed to avoid some graphicial odities in the diagram. Without the strip if either string is a multiline string (defined with | as in the examples) a traling newline is added which causes issues when the fields are joined together in the cell. Most of the other cells ignore this problem as the trailing
is ignored by graphvis.

removing them and generating the diagram gives

with mini bom

without mini bom

The alternatives could be either getting user to use the |- symbol instead (should update the docs in this case). or moving the cleanup of the <br />,, <br />) and <br /><br /> into the render code for the additional components cells.

[formatc1702]

In this case I'd argue having that little .rstrip() is nicer than requiring |- in the input YAML.

[formatc1702]

My question is, where does the need for .capitalize() come from?
Perhaps I'm missing something, but I don't see a need to mess with the user-defined type attribute. This applies to all instances of this function call.

[Tyler-Ward]

The capitialise was originaly added to create more consistent bom formatting. We can remove this and pass that requirment onto the user if they care about it as this would be fairly intuative for a user to know what to change (unlike the newlines).

[kvid]

I agree with your decision to remove the .capitalize(). Personally, I think I would have preferred recommending |- in the doc before removing the trailing newline with .rstrip(), but I understand that others might have a different opinion. The only motivation for a user to use | when specifying this attribute is to insert newlines, and then it shouldn't be too much of a surprise seeing the results above. If we in the Multiline section of the doc write "recommend |- to avoid a trailing newline", that should be enough, IMHO, but I respect that you disagree. No hard feelings. 😄

[kvid]

Did you reject this suggestion or just didn't see it?

[kvid]

I suggest inserting a single space after if as in all the other if-statements:

Suggested change

	if(entry['item'], entry['unit'], entry['manufacturer'], entry['mpn'], entry['pn']) == target:
	if (entry['item'], entry['unit'], entry['manufacturer'], entry['mpn'], entry['pn']) == target:

[kvid]

I suggest reversing your latest changes here to remove the .strip() call:

Suggested change

	rows.append(component_table_entry(f'#{id} ({extra.type.capitalize().strip()})', qty, extra.unit))
	rows.append(component_table_entry(f'#{id} ({extra.type.capitalize()})', qty, extra.unit))

• I assume this was one of the things you tried to avoid space before comma in the BOM.
• Now, when you remove those spaces in clean_whitespace(), no such actions should be needed here.
• The only reason for the user to include newlines, is to adapt the text in the diagram, and then we shouldn't remove it in the diagram without a very good reason. It's better to let the user see the consequence if it was a mistake, and then he can remove it from the input.

[kvid]

I suggest removing the no longer used functions graphviz_line_breaks() and remove_line_breaks() above.

[formatc1702]

Any reason the Literals are commented out?

[Tyler-Ward]

from @kvid's comment #115 (comment)

Note that the Literal type specifier is in comments to avoid requiring Python 3.8.

I have checked my 2.7 install and literals aernt supported, doing some searching I cant find any reference to this method of specifying literals so not sure if this is a standard implementation method or not.

[formatc1702]

Understood. Let's keep it like this then, perhaps add a comment behind the comment explaining this.

[kvid]

New in version 3.8 is specified by https://docs.python.org/3/library/typing.html#typing.Literal

[Tyler-Ward]

Thanks for the additional suggestions, I believe all of them have now been implemented or decided against in the inline comments but let me know if I have missed any.

[formatc1702]

Once there are no further comments, I will likely squash this PR into a single commit, unless @Tyler-Ward wants to go through the trouble (not worth it IMHO) of compressing the current 30 commits into something more manageable with meaningful intermediate steps.
@kvid since you've been keeping an eye on the implementation, I will rely on your GO to proceed with the squash+merge.

[kvid]

Thank you for accepting more suggestions. As far as I can see now, you have either implemented or decided against all my suggestions. I provided a link to the documentation of Literal that you couldn't find in that code comment thread, and explained why I would prefer recommending |- before calling .rstrip(), but that I also respect your opinion.

Thank you for the fantastic effort to develop this PR with the useful feature and improving the BOM code in the process. I'm also impressed by your patience to process my numerous suggestions about details.