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

test: Add init manifest script #66

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

kasbah
Copy link
Collaborator

@kasbah kasbah commented Jun 19, 2018

No description provided.

Copy link
Member

@mcous mcous left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've come around to this. Turns out it was 12AM (or whatever) and I wasn't thinking about the benefits of lowering the barrier to entry for adding more integration tests, so I'm sorry about that.

If you don't mind, I've got a few housekeeping requests:

  1. Rename it to init-board-manifest.js
    • We may want to add other scripts to the fixtures
    • .js extension ensures the linter will pick it up
  2. Put init-board-manifest.js in fixtures/bin and wire it up in fixtures/package.json
    • {"bin": {"init-board-manifest": "bin/init-board-manifest.js"}}
    • That way, yarn run init-board-manifest from the top level will be a thing

Then, to address the stuff I brought up in #64, can we more explicitly print out (a prompt would be amazing, but an interactive CLI is a much bigger project) what each setting was for each file? Something like:

please verify the following layer settings are correct

arduino-uno.cmp
> file type:   gerber
> layer type:  top copper

Looks like the current script relies on stdout piping, so that stuff could be fine to send to stderr via console.warn or something

@mcous mcous closed this Mar 9, 2019
@mcous mcous changed the base branch from next to master March 9, 2019 20:33
@mcous
Copy link
Member

mcous commented Mar 9, 2019

Oops, closed automatically when I deleted next. Sorry!

@mcous mcous reopened this Mar 9, 2019
@codecov
Copy link

codecov bot commented Mar 9, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@694eb66). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #66   +/-   ##
=========================================
  Coverage          ?   88.57%           
=========================================
  Files             ?       55           
  Lines             ?     2180           
  Branches          ?        0           
=========================================
  Hits              ?     1931           
  Misses            ?      249           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 694eb66...10550d0. Read the comment docs.

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.

None yet

2 participants