Initial commit of feature-complete v1 version #1
Conversation
README.md
Outdated
| @@ -0,0 +1,17 @@ | |||
| # DigiKey-Reports | |||
| An actions repository for DigiKey API integration and report generation given an input BOM | |||
There was a problem hiding this comment.
It might be worth linking to the generate BOM action to show how the BOM input can be generated.
There was a problem hiding this comment.
That's a good idea. Let's add that to the README.
|
|
||
|
|
||
| ################################################################################ | ||
| class ComponentData: |
There was a problem hiding this comment.
This could be a @dataclass, which means you don't have to define __init__ here and then mutate the object later.
There was a problem hiding this comment.
That's a good point, I'll add the decorator.
| response.json()["access_token"] if response.status_code == 200 else None | ||
| ) | ||
| # Return response status code and access token | ||
| return (response.status_code, access_token) |
There was a problem hiding this comment.
I don't see this response code being used anywhere. Is there a reason we're returning it?
There was a problem hiding this comment.
Good catch, the response code is being used in the API query endpoint but not in the auth. I'll add the check to auth to exit if auth fails.
entrypoint.py
Outdated
| # Unzip the JS/CSS assets | ||
| shutil.unpack_archive("/report_template/assets.zip", "/component_report") | ||
| # Write HTML output file | ||
| with ExitStack() as stack: |
There was a problem hiding this comment.
You don't need an exit stack here, you can directly use:
with open(...) as report_file:There was a problem hiding this comment.
Do we need to keep these files here and in the zip? I think just the zip should be enough?
There was a problem hiding this comment.
Agree, let's can the assets folders and keep only the zip
report_template/index.html
Outdated
| {% if part.mfr_name is not none %} | ||
| <div class="row g-0"> | ||
| <div class="col-auto text-center" style="padding-right: 20px;padding-left: 20px;border-style: none;border-color: var(--bs-gray-400);"><img src="{{ part.photo_url }}" style="height: 88px;margin-left: 10px;"><a href="{{ part.datasheet_url }}" target="_blank"> | ||
| <p style="text-align: center;font-size: 10px;padding-bottom: 5px;"><i class="far fa-file-pdf" style="font-size: 20px;color: var(--bs-form-invalid-color);"></i></p> |
There was a problem hiding this comment.
We're only using font awesome for this one icon AFAICT, is it worth adding all of its assets for this?
There was a problem hiding this comment.
I'm able to replace the font awesome full library with just the SVG I need for that icon - will make the change.
.github/workflows/test.yml
Outdated
There was a problem hiding this comment.
Rename test.yml to semantic meaning. What is this workflow doing? Name the file that.
There was a problem hiding this comment.
Good catch. It's named as test.py in generate-bom and cofactr-cogs also - we will prolly want to rename those as well. It's a ruff format/lint check workflow - we should rename it as that.
…BOM generation expectation and link, ComponentData declared as a dataclass, API authentication failure handled with graceful exit, ExitStack usage removed, report template assets folder removed, font awesome icon implemented as standalone SVG and fonts removed.
|
Implemented feedback changes, ready to merge. |
@shrik450 could you help to take a quick look and approve the PR please? :-)