Hide files with given glob patterns #150
Conversation
|
Thanks for the PR! Reviewing this is in my todo list, I'll get to it when I can. :D |
lib/models.js
Outdated
| @@ -58,6 +58,16 @@ export class Path { | |||
| return this.stat ? !this.stat.isDirectory() : null; | |||
| } | |||
|
|
|||
| isVisible() { | |||
| ignoredFiles = config.get('ignoredFiles'); | |||
There was a problem hiding this comment.
We want to declare variables with const if they don't change, or let if they do.
lib/config.js
Outdated
| @@ -45,5 +45,11 @@ export let config = { | |||
| details.`, | |||
| type: 'boolean', | |||
| default: false, | |||
| }, | |||
| ignoredFiles: { | |||
There was a problem hiding this comment.
ignoredExtensions is a more accurate name for this config setting, since the value doesn't contain files, but extensions.
There was a problem hiding this comment.
renamed to ignoredPatterns since it now uses glob patterns.
lib/config.js
Outdated
| ignoredFiles: { | ||
| title: "Ignore files with these extensions", | ||
| description: "Don't show files with the given extensions.", | ||
| type: 'string', |
There was a problem hiding this comment.
Atom allows for array config values, which would be appropriate here.
There was a problem hiding this comment.
Changed to array of string
lib/models.js
Outdated
| ignoredFiles = config.get('ignoredFiles'); | ||
| if (!ignoredFiles) return true; | ||
| ignoredExtensions = ignoredFiles.split(','); | ||
| for( i in ignoredExtensions) { |
There was a problem hiding this comment.
A for...of loop would work nicely here.
lib/models.js
Outdated
| if (!ignoredFiles) return true; | ||
| ignoredExtensions = ignoredFiles.split(','); | ||
| for( i in ignoredExtensions) { | ||
| if(this.fragment.endsWith(ignoredExtensions[i])) return false; |
There was a problem hiding this comment.
I'm a little torn here, because this isn't strictly extension matching, it's suffix matching. Which is fine, but then the natural next step is to add prefix matching, etc. Maybe a list of regexes to check against the file?
Also, it might be more performant to add this ignoring logic to the matchingPaths method instead of adding isVisible, which would avoid creating a new Path object for paths that aren't going to be displayed.
There was a problem hiding this comment.
I applied these suggestions in the last commit: using globs (which I find more natural than regexp for filename); moved to the matchingPaths method; compile one and cache regexps for performances.
econtal
changed the title
|
Thanks for the suggestions. I updated the PR which now uses glob patterns instead of suffixes. It uses minimap as a backend to compute the matching. The Minimap objects are compiled once and then cached to optimize performances. |
|
Hi @Osmose: please let me know, when you have time, if you have other suggestions! |
lib/utils.js
Outdated
| * the regexp. | ||
| */ | ||
| export function ignoredPatterns() { | ||
| if (this.minimatches === undefined ) { |
There was a problem hiding this comment.
I'm unsure what this is set to when the function is imported into another file. Is it the global for this module?
In any case, just adding a let _minimatches = null; above the function and using that instead would clear that confusion up.
There was a problem hiding this comment.
Also, this caching means that changes to the preference wouldn't take effect in existing windows. I can think of two solutions to this:
- Make the cache and object keyed by the glob patterns, and re-fetch the config each time. That way we always get the current config, but can cache minimatch objects.
- Add an observer for changes to the config and invalidate the cache when the config changes.
Either would be fine IMO. The former seems easier.
| "osenv": "^0.1.3", | ||
| "touch": "^1.0.0" | ||
| "touch": "^1.0.0", | ||
| "stringmap": "~0.2.2" |
There was a problem hiding this comment.
Note: stringmap is already used a dependencies of dependencies, so this shouldn't increase load time
|
Sorry for this delay before updating the PR. I appreciated the above comments and did my best to address them. Thanks for the ideas! |
|
Thanks for this! I realized I totally forgot to mention tests and will add one or two before merging this, and will do a release shortly after. |
|
Nice feature! |
This MR allows to hide certain files in the list, using their extension. Example: hides
.pycfiles.