-
Notifications
You must be signed in to change notification settings - Fork 229
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
Feature/short bom names #121
Conversation
internal part number is now refered to as part number in bom
Tutorial changed to use short parn number field names Add aditional bom items to example
Looks good! You have my vote for the second option (merged fields). One minor thing: since we've moved P/N in front of the manufacturer info, let's do the same in the BOM, for consistency. Also, the exported BOM header still has the long names, please shorten. Thanks! |
src/wireviz/wv_helper.py
Outdated
part_number_component = f': {mpn}' if mpn else '' | ||
return(f'{manufacturer}{part_number_component}') | ||
else: | ||
return(f'MPN: {mpn}' if mpn else None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about this?
if manufacturer or mpn:
return f'{manufacturer if manufacturer else "MPN"}{": " + str(mpn) if mpn else ""}'
else
return None
It should return the same four options:
None
(nothing specified)
{manufacturer}
(no MPN specified)
MPN: {mpn}
(no manufacturer specified)
{manufacturer}: {mpn}
(both specified)
IMHO a bit cleaner and easier to understand.
The str()
functions allows using +
to concatenate strings without errors if mpn
is passed as an int
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@formatc1702 How about this alternative instead?
if manufacturer:
return f'{manufacturer}: {mpn}' if mpn else manufacturer
else:
return f'MPN: {mpn}' if mpn else None
Edit: It's closer to the original, except without a variable and without parentheses around the return expressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That looks like a much tider way to implement that, have swapped to that implementation in the latest commits
new order is part number manufacturer manufacturer part number
Also update BOM header generation to support specific headers for fields were capitalize produces unwanted output.
The changes in the above comments have been added, let me know if there are any other edits wanted. |
Fixes issue wireviz#129, but originally suggested in review of wireviz#121.
Fixes issue wireviz#129, but originally suggested in review of wireviz#121.
This implements #114 commit b97e0ea implements the medged manufacturer and mpn fields examples of each are shown below this can be included or excluded from the merge depending on peoples opinion on which version looks better.
Seperated fields:
Merged fields: