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.

Sign up for GitHub

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

Jump to bottom

Implement suppliers and supplier part numbers #241

Merged
merged 12 commits into from
Sep 14, 2021
Merged

Implement suppliers and supplier part numbers #241

merged 12 commits into from
Sep 14, 2021

Conversation

formatc1702
Copy link
Collaborator

Closes #240.

@formatc1702
Add supplier and spn fields
2d5a3c6
@formatc1702
Update tutorial08
0c1e729
@formatc1702
generalize MPN/SPN string generation
6e59163 
rename `manufacturer_info_field()` to `pn_info_string()` and add parameter to specify MPN or SPN
@formatc1702 formatc1702 changed the base branch from master to dev August 27, 2021 16:58
kvid
kvid requested changes Aug 29, 2021
View reviewed changes
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.

Except for one bug description, this review mainly suggests you consider a few code alternatives with no change of output.

src/wireviz/Harness.py Outdated Show resolved Hide resolved
src/wireviz/Harness.py Outdated Show resolved Hide resolved
src/wireviz/Harness.py Outdated Show resolved Hide resolved
src/wireviz/Harness.py Outdated Show resolved Hide resolved
src/wireviz/Harness.py Outdated Show resolved Hide resolved
src/wireviz/wv_bom.py Outdated Show resolved Hide resolved
src/wireviz/wv_bom.py Outdated Show resolved Hide resolved
src/wireviz/wv_bom.py Show resolved Hide resolved
tutorial/tutorial08.yml Show resolved Hide resolved
src/wireviz/wv_bom.py Outdated Show resolved Hide resolved
@formatc1702 formatc1702 requested a review from kvid August 29, 2021 09:41
kvid
kvid requested changes Sep 1, 2021
View reviewed changes
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.

Thank's for implementing most of my suggestions in my previous review. However, there are a few issues that I forgot to mention.

src/wireviz/wv_bom.py Outdated Show resolved Hide resolved
src/wireviz/wv_bom.py Outdated Show resolved Hide resolved
src/wireviz/wv_bom.py Outdated Show resolved Hide resolved
tutorial/tutorial08.yml Show resolved Hide resolved
@formatc1702 formatc1702 requested a review from kvid September 8, 2021 16:01
@formatc1702 formatc1702 merged commit a3eefe6 into dev Sep 14, 2021
@formatc1702 formatc1702 deleted the feature/spn branch September 14, 2021 17:30
formatc1702 added a commit that referenced this pull request Sep 14, 2021
@formatc1702
Add #240, #241
32a655c
formatc1702 added a commit that referenced this pull request Jun 7, 2023
@formatc1702
Add #240, #241
602427c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kvid kvid kvid approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[feature] Add supplier / SPN info
2 participants
@formatc1702 @kvid