-
Notifications
You must be signed in to change notification settings - Fork 100
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
fix: Do not show Errors from reading directories #1717
fix: Do not show Errors from reading directories #1717
Conversation
1502f62
to
d96790f
Compare
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 works well (yay! thanks!) but I think I'd rather have the issue fixed closer to its source. What do you think?
I'm not against merging as-is if you think what's currently in the PR is a better approach, especially since this repo is old and relatively inactive.
@@ -16,6 +16,10 @@ export const convertFileComments = async ( | |||
) => { | |||
const fileContent = await dependencies.fileSystem.readFile(filePath); | |||
if (fileContent instanceof Error) { | |||
if (fileContent.code === "EISDIR") { | |||
return undefined; | |||
} |
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.
[Structure] I actually think the approach here isn't what I'd prefer. collectCommentFileNames
is taking in a typescriptConfiguration
that looks like:
{
"compilerOptions": { "target": "es3" },
"files": ["./cool/index.ts"],
"include": ["cool"]
}
...but with an output commentFileNames
like:
{
exclude: undefined,
include: [ './cool/index.ts', 'cool' ]
}
(note that the original TSConfig that made that is just { "include": ["cool"] }
.
I think the right approach to fix this earlier in the system would be to have collectCommentFileNames
filter out directories from its output. Or maybe just not use the "include"
at all, if files
has the full list of files?
What do you think?
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.
That sounds good! I have changed things so now collectCommentFileNames
handles the filtering of directories. Sorry about the slow response/changes
d96790f
to
d2f327b
Compare
d2f327b
to
b4b2ce9
Compare
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.
Swell, thanks! This is looking great - and I appreciate you refactoring the internals. 🙌
I'll apply a couple of nits that honestly don't really change much.
.map((isDirectory, i) => { | ||
if (isDirectory) { | ||
return null; | ||
} | ||
return includeList[i]; | ||
}) |
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.
Nit: could one-line this if you really wanted 😄
.map((isDirectory, i) => { | |
if (isDirectory) { | |
return null; | |
} | |
return includeList[i]; | |
}) | |
.map((isDirectory, i) => !isDirectory && includeList[i]) |
Oh, I don't have permissions to modify your fork. No worries - it's just nits. |
Published in |
PR Checklist
status: accepting prs
Overview
If a
tsconfig.json
contains something like the following:Then, an error shows up, for example, when using
--comments
:According to the TypeScript docs, a directory entry for
include
is valid:This fixes that case.