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

Feature/short bom names #121

Merged
merged 6 commits into from
Jul 26, 2020
Merged

Conversation

Tyler-Ward
Copy link
Contributor

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:
ex02

Merged fields:
ex02

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
@formatc1702
Copy link
Collaborator

formatc1702 commented Jul 23, 2020

Looks good! You have my vote for the second option (merged fields).
I'll try it out as soon as I can.

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.
PN | Manufacturer | MPN

Also, the exported BOM header still has the long names, please shorten.

Thanks!

Comment on lines 124 to 128
part_number_component = f': {mpn}' if mpn else ''
return(f'{manufacturer}{part_number_component}')
else:
return(f'MPN: {mpn}' if mpn else None)
Copy link
Collaborator

@formatc1702 formatc1702 Jul 23, 2020

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.

Copy link
Collaborator

@kvid kvid Jul 23, 2020

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.

Copy link
Contributor Author

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

@formatc1702 formatc1702 added this to the v0.2 = PRIORITY milestone Jul 24, 2020
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.
@Tyler-Ward
Copy link
Contributor Author

The changes in the above comments have been added, let me know if there are any other edits wanted.

@formatc1702 formatc1702 merged commit b9a4783 into wireviz:dev Jul 26, 2020
kvid added a commit to kvid/WireViz that referenced this pull request Jul 27, 2020
Fixes issue wireviz#129, but originally suggested in review of wireviz#121.
kvid added a commit to kvid/WireViz that referenced this pull request Jul 28, 2020
Fixes issue wireviz#129, but originally suggested in review of wireviz#121.
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.

3 participants