Skip to content

fix issue #6861, ignore hidden files by default #6894

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

Closed
wants to merge 1 commit into from
Closed

fix issue #6861, ignore hidden files by default #6894

wants to merge 1 commit into from

Conversation

HerringtonDarkholme
Copy link
Contributor

Fix #6861

I'm new to TypeScript compiler, please forgive my ignorance.

There are two places I modified: finding files when tsc start and detecting file changes.

isHiddenFile is currently unixy only. I don't think the same logic applies on windows.

@msftclas
Copy link

msftclas commented Feb 4, 2016

Hi @HerringtonDarkholme, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@@ -769,6 +769,10 @@ namespace ts {
return pathLen > extLen && path.substr(pathLen - extLen, extLen) === extension;
}

export function isHiddenFile(path: string): boolean {
return getBaseFileName(path)[0] === "."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

; missed

@DanielRosenwasser
Copy link
Member

I think the functionality offered by #5980 is more general and should be preferred to adding special cases in the compiler.

@HerringtonDarkholme
Copy link
Contributor Author

@DanielRosenwasser I think so. Should this pull request be closed for now and wait for #5980 merged?

@HerringtonDarkholme
Copy link
Contributor Author

wait for #5980

@msftclas
Copy link

Hi @HerringtonDarkholme, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@zhengbli
Copy link
Contributor

zhengbli commented Apr 26, 2016

Hi @HerringtonDarkholme , we should be able to implement this fix without the glob feature for now. Do you still want to work on this? If so, you probably want to exclude the dotted folders as well, for example .git, .vscode etc. If not, I am happy to take over. Thank you!

P.S. for folder exclude, you can take a look at how we exclude node_modules folder by default here

@HerringtonDarkholme
Copy link
Contributor Author

I'm happy to implement this! I will try to submit my pull request by next three day. If I'm too busy to implement it, I'll drop a comment here to transfer this feature to @zhengbli .

@HerringtonDarkholme
Copy link
Contributor Author

After re-reading the code I still think glob is a more appropriate approach and is more requested feature. Adding a special case in compiler is not elegant indeed, adding common exclusion like .gitignore, .vscode is the best I can see... @zhengbli any suggestion? Should I just keep the current implementation and add more folder exclusion?

@DanielRosenwasser
Copy link
Member

@zhengbli the current implementation, as I understand it, already ignores dotted folder names, so this should be fine, shouldn't it?

@zhengbli
Copy link
Contributor

@DanielRosenwasser I think this PR only checks if the base file name, which doesn't include the folder name, starts with a dot or not. So I don't think the dotted folders are covered. I also tested the latest nightly, dotted folders are included as expected.

@HerringtonDarkholme You are right that the implementation would be more elegant with support of glob patterns. However, as that PR may still need quite some time before merging in, and this change looks like a small one, my vote is to implement as is for now, and we can change the implementation later.

Plus, I don't think we should "enforce" the exclusion in compiler/core.ts. If the user specified a dotted folder, we should respect that and override the default implicit exclusion.

@HerringtonDarkholme
Copy link
Contributor Author

I still don't think ignoring hidden directory can be done without modifying host.readDirectory or hardcoding core.ts. If I select the first approach then the RR will become large which we want to avoid at first place since #5980 is a more general solution.

Also, I'm quite busy with my daily work. Transferring this to @zhengbli is better.

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants