Bring project back to life #26
Conversation
|
@zimme Ready to review and merge-ready I suggest to read the commit messages and maybe check commit by commit to understand what I have done 🙂 Tests still failing, but didn't touched them so much If you have any questions, feel free to ask |
|
I'll try my best to review this tomorrow |
| @@ -4,39 +4,35 @@ | |||
| "url": "https://github.com/linterjs/core/issues" | |||
| }, | |||
| "dependencies": { | |||
| "common-tags": "^1.8.0", | |||
There was a problem hiding this comment.
Using ~ will only allow PATCH version updates, while ^ will allow MINOR version updates. I guess I'm ok with only allowing PATCH updates, however if this package will be using @linter/cli in the future that will have to be using ^ as our packages will never have changes to PATCH as that will always be 0.
There was a problem hiding this comment.
Oh sorry! you are right, but only for dependencies.
But we should use ~ for devDependencies
I'm always using ~ but I'm using them in frontend projects, and these are not dependencies for other projects ^^
| branches: 100, | ||
| functions: 100, | ||
| lines: 100, | ||
| statements: 100, |
There was a problem hiding this comment.
If this change was to fix non passing tests, let's try and fix the tests later so we get 100 coverage as the only thing I actually want to test is this package's API and all permutations of using the API. I don't want to have tests of implementation details.
There was a problem hiding this comment.
It was not my aim to fix test passing but 100% coverage is something that sometimes cannot be possible at all
A coverage of 85%-95% is totally valid
Lines like https://github.com/linterjs/core/blob/master/src/errors.ts#L4-L7 wont be covered (currently) 🤔
No description provided.