meszaros-lajos-gyorgy / minimist-lite

parse argument options
Other
15 stars 3 forks source link

ci: add 'continuous integration' & 'publish' github workflows #20

Closed micalevisk closed 2 years ago

micalevisk commented 2 years ago

closes #15

I've added one workflow to run tests and another to just publish. Is it fine?


Also, the ci.yml will fail for now because npm run test exits with non-zero code due to code coverage report constraints, and I don't believe we should relax that.

image

image

image

image

micalevisk commented 2 years ago

btw @meszaros-lajos-gyorgy what's the reason the following is nested under a try-catch clause? will Number(x) or Number.MAX_SAFE_INTEGER raise an error in some case?

https://github.com/meszaros-lajos-gyorgy/minimist-lite/blob/50bfec5cd161af8499a51a6ebfcf3ef3e0889a69/src/functions.js#L4-L8

meszaros-lajos-gyorgy commented 2 years ago

Hi! Sorry for the late respone, had a busy week. Let's have a test run, I'm curious to see if this works properly.

meszaros-lajos-gyorgy commented 2 years ago

I think we should introduce a development branch so that we can have a controlled release procedure and not release a new version on every commit. What do you say?

micalevisk commented 2 years ago

the release workflow is triggered by new git tags starting with v, isn't this good?

meszaros-lajos-gyorgy commented 2 years ago

btw @meszaros-lajos-gyorgy what's the reason the following is nested under a try-catch clause? will Number(x) or Number.MAX_SAFE_INTEGER raise an error in some case?

https://github.com/meszaros-lajos-gyorgy/minimist-lite/blob/50bfec5cd161af8499a51a6ebfcf3ef3e0889a69/src/functions.js#L4-L8

I have no idea why this is in a try catch cause, it was probably refactored back then from a code, which threw an error upon parsing a number. I think it can be removed, but let me double check that number constructor.

meszaros-lajos-gyorgy commented 2 years ago

the release workflow is triggered by new git tags starting with v, isn't this good?

Ah, okay, that is perfect!

micalevisk commented 2 years ago

sure. Then you can open a PR to test the CI workflow

The issue is that the code coverage isn't 100% so it will fail

meszaros-lajos-gyorgy commented 2 years ago

Checked the Number constructor and nowhere does it mentions that it might throw an error: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/Number Parsing something, which is not a number will result in NaN, which can be compared with numbers without throwing an error.

meszaros-lajos-gyorgy commented 2 years ago

I think I never saw the coverage report being 100% before and I would be perfectly happy with around 90% check, but if you say that we should aim for 100% then I can support that too.

micalevisk commented 2 years ago

@meszaros-lajos-gyorgy what about

  "tap": {
    "branches": 95,
    "lines": 80,
    "functions": 100,
    "statements": 80
  },

you can try it locally by adding the above in your package.json, and run npm t

meszaros-lajos-gyorgy commented 2 years ago

Let's go with this!

micalevisk commented 2 years ago

would you like to made that change along with the refactoring of that isNumber func? it would be even better if you manage improve the code coverage threshold

Also, I think is better to move the tap config above to a new .taprc file (it's a JSON file).

meszaros-lajos-gyorgy commented 2 years ago

We should have a separate ticket for all of these small changes. Yes, I can implement these changes.