New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for directory creation to --output-file flag #5672
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dreyks Thanks for creating this patch quickly! Can you add a test case?
For example, it seems good to add simple test cases: // lib/__tests__/writeOutputFile.test.js
'use strict';
const writeOutputFile = require('../writeOutputFile');
describe('writeOutputFile', () => {
it('creates a file', () => {
// ...
});
it('creates a directory if it does not exist', () => {
// ...
});
}); |
should this test write to the real FS or do we want to add memfs or similar? |
Can you use the real FS? |
i can. i'll just clean up tmp files after each test |
lib/__tests__/outputFile.test.js
Outdated
|
||
expect((await fs.readFile(filePath)).toString()).toEqual('test content'); | ||
|
||
await fs.rmdir(path.dirname(filePath), { recursive: true }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks a bit concerning, maybe I should replace this with
await fs.unlink(filePath)
await fs.rmDir(path.dirname(filePath))
so that there's no way to accidentally remove more than we expect :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! LGTM 👍🏼
i think i still need a second approval to merge this. do I need to do anything else or is the process now on your (stylelint maintainers) side? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dreyks Thanks for the pull request. LGTM.
were removed in a big refactoring PR
Oops, removing them was probably a mistake by me...
do I need to do anything else or is the process now on your (stylelint maintainers) side?
It's on our side.
Changelog entry added:
|
Closes #5670
Looks like the test-files that were a part of the original PR were removed in a big refactoring PR so I haven't added any