Add GitHub Actions reporter #1058
Conversation
| @@ -0,0 +1,66 @@ | |||
| import core from '@actions/core'; | |||
There was a problem hiding this comment.
@webpro Do we want to depend on GitHub Actions as a dependency of Knip? That seems like a lot of overhead for a single log reporter. If not, we could probably easily reverse-engineer the Actions logger. The syntax is pretty simple.
There was a problem hiding this comment.
Yeah I was just happy to reduce deps to this: https://npmgraph.js.org/?q=knip
On the other hand, I don't know what it involves to replicate that logger?
There was a problem hiding this comment.
When i did the initial draft of the reporter, i tried to replicate it using console.log and it did not work.
I think its ok marking actions/core as a peer dependency and just document that when using github actions reporter, @actions/core must also be installed.
There was a problem hiding this comment.
I've moved it to optionalDependencies, because I personally am not a fan of using peers for things that are, well.. optional. Thoughts?
There was a problem hiding this comment.
you're right, optional is the best case here
There was a problem hiding this comment.
Perhaps I'm exaggerating after trying to cut down on Knip's package size, but looking at their implementation I do have mixed feelings about adding this dependency. An optional dependency is still a dependency.
Even though it's certainly isn't trivial and precision with custom output syntax is required, but essentially we're using three methods and they're all logging to the stdout. Is there any chance you could implement/borrow only the necessary parts?
There was a problem hiding this comment.
Yeah, sounds good to me. I would be similarly hesitant, which is why I asked. I haven't had a chance to test this against my CI yet (which is why it's still a draft), but I'll try to reimplement it simply to avoid adding the dependency once I do get a chance.
There was a problem hiding this comment.
optionalDependencies are still installed, even if they don't show up on npmgraph:
Details: https://docs.npmjs.com/cli/v11/configuring-npm/package-json#optionaldependencies
If you don't want it to be a dependency locally you have a few options:
- make it a plugin (only people who use it install it) — not great
- offer knip as an action, so it can have its own deps
- install it on the fly when you detect the GHA environment. This might be troublesome in practice.
There was a problem hiding this comment.
If the action took just took in a json report like https://github.com/ataylormet/eslint-annotate-action does, then the core of knip itself wouldn’t need the action dependency.
Disregard, I see that this may not even need that level of complexity for basic annotation.
| @@ -0,0 +1,66 @@ | |||
| import core from '@actions/core'; | |||
There was a problem hiding this comment.
Yeah I was just happy to reduce deps to this: https://npmgraph.js.org/?q=knip
On the other hand, I don't know what it involves to replicate that logger?
| @@ -0,0 +1,32 @@ | |||
| import core from '@actions/core'; | |||
| import { ISSUE_TYPES, ISSUE_TYPE_TITLE } from 'src/constants.js'; | |||
There was a problem hiding this comment.
| import { ISSUE_TYPES, ISSUE_TYPE_TITLE } from 'src/constants.js'; | |
| import { ISSUE_TYPES, ISSUE_TYPE_TITLE } from '../constants.js'; |
There was a problem hiding this comment.
As in my original draft, you cannot use ISSUE_TYPES directly because it has files and not _files.
There was a problem hiding this comment.
I see that the disclosure reporter works around this like so:
Should I just copy that pattern?
|
|
||
| const cwd = resolve('fixtures/module-resolution-non-std'); | ||
|
|
||
| test('knip --reporter githubActions (files, unlisted & unresolved)', () => { |
There was a problem hiding this comment.
I guess would be great to use the github actions reporter in ci.yml since this is github :)
EzraBrooks
force-pushed
the
add-github-actions-reporter
branch
from
2eb9c6b to
8a881b4
Compare
webpro
force-pushed
the
main
branch
5 times, most recently
from
83dfdce to
61d09ab
Compare
|
@EzraBrooks any updates on the review? I can work on it now if you give me permissions to your fork :) |
|
Hey, thanks for offering - I got pulled into something last-minute that is taking up basically my entire week this week, sorry about that. Feel free to just cherry-pick my commit elsewhere and amend it, if you'd like. |
webpro
force-pushed
the
main
branch
2 times, most recently
from
ea72d80 to
7787123
Compare
webpro
force-pushed
the
main
branch
4 times, most recently
from
5a2d249 to
6bd250a
Compare
Closes #1012.
Still WIP, but opening a draft so I make sure to get it done :)