Updated the new actions toolkit and fixed multiple check issue #17
Conversation
|
The issue I was having in #13 will be fixed on xo merges xojs/xo#405 |
There was a problem hiding this comment.
This looks great ✨, @OmgImAlexis
Left a few comments still.
| const fixXoForChildProcess = async () => { | ||
| const cliMainPath = path.join(path.dirname(fs.realpathSync(xoPath)), 'cli-main.js') | ||
| fs.writeFileSync(cliMainPath, fs.readFileSync(cliMainPath).toString().replace('process.exit(report.errorCount === 0 ? 0 : 1);', 'process.exitCode = (report.errorCount === 0 ? 0 : 1);')); | ||
| }; |
There was a problem hiding this comment.
Not sure what this is used for?
@OmgImAlexis, can you explain, please?
There was a problem hiding this comment.
It's a patch for this PR.
There was a problem hiding this comment.
Cool.
Can you add a comment that it is a workaround for the linked PR and could be removed if it gets merged, please.
| lint: | ||
| name: Lint code | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@master | ||
| - name: Install deps | ||
| uses: docker://node:10-alpine | ||
| with: | ||
| entrypoint: npm | ||
| args: ci | ||
|
|
||
| - name: Lint | ||
| uses: docker://node:10-alpine | ||
| with: | ||
| entrypoint: npm | ||
| args: run lint |
There was a problem hiding this comment.
@OmgImAlexis, what do you think changing this to
lint:
name: Lint code on Node v${{ matrix.node }}
runs-on: ubuntu-latest
strategy:
matrix:
node: ['10', '12', '13']
steps:
- uses: actions/checkout@master
- uses: actions/setup-node@master
with:
node-version: ${{ matrix.node }}
- run: npm ci
- run: npm run lintSo it can be "tested" agains other Node versions?
There was a problem hiding this comment.
Does it really make sense to run the linter action against a matrix? If there were test cases to check the linter execution against a matrix it would be fine. But this is actually just ensuring the linter code does not have lint issues, so node versions should not matter, if they do, this would mean code issues and thus require proper test cases.
|
This issue/pr has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions. |
|
I'll try and get to this soon. Got stuck on other things at work. |
|
This issue/pr has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions. |
stale
bot
added
🤖 stale
|
Sorry to step into the discussion, but I am interested in this action and it is currently not working, so I hope to provide some feedback and maybe help fix it if that's ok. I tried to use the current release version and faced the JSON.parse error that is mentioned in this issue: #13 Then I tried to test my project against this PR to see if that would fix the issue. This brought up two problems: 1 - The linter does not give any output regarding which errors it found 2 - One of the action processes got stuck forever running |
I'd recommend squashing this commit as there are quite a few extra commits that were adding just while testing.
Changes in this PR
Closes #13
Have you ...
cc/ @stoe