Improve some more code #1892
Improve some more code #1892
Conversation
Similar to TypeStrong#1889 — improves some code to make it more readable.
| @@ -279,7 +277,7 @@ export class SourceLinkPlugin extends ConverterComponent { | |||
| // No repository found, add path to ignored paths | |||
| const segments = dirName.split("/"); | |||
| for (let i = segments.length; i > 0; i--) { | |||
| this.ignoredPaths.push(segments.slice(0, i).join("/")); | |||
| this.ignoredPaths.add(segments.slice(0, i).join("/")); | |||
There was a problem hiding this comment.
Without a set, this could add sub-paths multiple times.
@Gerrit0 My understanding of this is that this would always ignore the entire path if a single file is ignored. Eg. if src/components/generated is in the .gitignore, this would ignore everything in src. I suspect that we'd rather just add the directory by itself, and not the path leading up to it.
There was a problem hiding this comment.
In other words, we might want to move this loop to the top of the function — the dirName should probably be checked if any part of it is ignored.
There was a problem hiding this comment.
this would always ignore the entire path if a single file is ignored. Eg. if
src/components/generatedis in the .gitignore, this would ignore everything insrc.
I read this the same way, and I agree, this definitely looks wrong. I'm surprised there haven't been any bug reports about it.
There was a problem hiding this comment.
I've added a fix for this issue.
| @@ -279,7 +277,7 @@ export class SourceLinkPlugin extends ConverterComponent { | |||
| // No repository found, add path to ignored paths | |||
| const segments = dirName.split("/"); | |||
| for (let i = segments.length; i > 0; i--) { | |||
| this.ignoredPaths.push(segments.slice(0, i).join("/")); | |||
| this.ignoredPaths.add(segments.slice(0, i).join("/")); | |||
There was a problem hiding this comment.
this would always ignore the entire path if a single file is ignored. Eg. if
src/components/generatedis in the .gitignore, this would ignore everything insrc.
I read this the same way, and I agree, this definitely looks wrong. I'm surprised there haven't been any bug reports about it.
Co-authored-by: Gerrit Birkeland <gerrit@gerritbirkeland.com>
|
Thanks! It'll be interesting to see if anyone notices differences from the source link plugin. Given that the previous broken behavior never got reported... I kind of suspect nobody will |
Similar to #1889 — updates some code to make it more readable.