minimistjs / minimist

parse argument options
MIT License
515 stars 30 forks source link

Avoid 0.6 tests due to build failures #20

Closed shadowspawn closed 1 year ago

shadowspawn commented 1 year ago

Node.js 0.6 does not install and PR checks always fail . Expand range to avoid the failing installation.

Fixes #19


An alternative would be to disable 0.6 upstream, or fix! I'm opening a PR for a local solution/work-around.

I could open another issue in minimist to reenable 0.6 if/when fixed upstream? Reference:

ljharb commented 1 year ago

I suppose this is fine, but I have this same thing on 300+ other projects and it's not much of a problem ¯\_(ツ)_/¯

codecov-commenter commented 1 year ago

Codecov Report

Merging #20 (ba92fe6) into main (950eaa7) will not change coverage. The diff coverage is n/a.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##             main      #20   +/-   ##
=======================================
  Coverage   98.75%   98.75%           
=======================================
  Files           1        1           
  Lines         161      161           
  Branches       71       71           
=======================================
  Hits          159      159           
  Misses          2        2           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

shadowspawn commented 1 year ago

I really dislike failing tests.

I feel either it is worth fixing now, or better as an open issue for long-term problems. Tracking ToDos or short-comings by failing tests is noisy and detracting from the checks, and in particular from my point of view it is mental overhead for people not familiar with the underlying issue.

I am more willing to accept noise from my own failing tests that I intend to fix, so I have some sympathy with your comment!

ljharb commented 1 year ago

That's what optional vs required is for :-) failing optional test are expected, failing required tests are a problem.

I am more than happy to see a fix in the action itself, but getting node 0.6 to compile on GHA is sadly not a simple task :-/

shadowspawn commented 1 year ago

That's what optional vs required is for :-) failing optional test are expected, failing required tests are a problem.

My complaint is identifying the difference. The optional/required does affect the email notifications, but I'm finding it hard to tell the difference in the web UX.

Here are three PR. One passes all tests, one has a failing "required" test, and one has a failing "optional" test.

Screenshot 2023-01-09 at 19 50 43
shadowspawn commented 1 year ago

(Woop! Thanks. I appreciate you are willing to do something different than your other projects. 👍 )

ljharb commented 1 year ago

Realistically this is a compelling argument to disable it in the shared action, until I can get it working, but I'll merge this now to make it easier on you in the meantime :-)