Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Make file stats available to indexFilter #12445

Merged
merged 1 commit into from
Dec 14, 2016
Merged

Conversation

d-akara
Copy link
Contributor

@d-akara d-akara commented May 22, 2016

Small change to make the file stat information available to the file filter.

There are extensions available to extend the filtering capability, but they are all limited in that it is difficult to have custom filtering for directories or file sizes etc without more information provided to the filter.

@ficristo
Copy link
Collaborator

IMHO, this raises the question if _indexFilter should be a public api.
Improving it only for the sake of extensions is seems to me a bit risky...

@d-akara
Copy link
Contributor Author

d-akara commented May 23, 2016

I agree about there should be discussion on if _indexFilter should be public.
I'm not sure I understand the risk statement. Maybe you could elaborate further to help me understand the concern?

@ficristo
Copy link
Collaborator

The risk is only from a maintanaince point of view.

@zaggino
Copy link
Contributor

zaggino commented Aug 25, 2016

@ficristo
I don't think this PR makes _indexFilter public. It's an underscored method which hints private and it's up to developers not to abuse it, this PR doesn't change that. From maintenance point of view, I'm sure @dakaraphi understands, that arguments passed to this function can change with any Brackets release and break his extensions. Voting for merge here.

@ficristo
Copy link
Collaborator

If the only motivation behind this change is for extensions sake, I'm against it.
IMHO we should spend 0 time on improving private API for that solely reason.
And if we start with one we create a precedent.
We should instead discuss if this kind of API should be made public and eventually add the necessary parameters.
That said, we can hear more opinions of others committers and decide what to do.

@d-akara
Copy link
Contributor Author

d-akara commented Aug 26, 2016

I just need a solution to provide this capability. All extensions which do filtering currently use this function. In order to get this functionality I must do a very ugly monkey patch in my extension.

Brackets is not usable in my project without this capability as it is a very complex directory structure with many hundred thousands of files. This certainly can stay private if Brackets wants to include such functionality by default.

For reference, my extension: https://github.com/dakaraphi/brackets-filter-extension

@ficristo
Copy link
Collaborator

If we are talking about filtering extensions, personally I always felt they were an hack done because of the limitations of Brackets.
The right fix is to provide these features natively in Brackets and not though an extension.
And I think, with the latest changes of @zaggino, we are almost there.

@zaggino
Copy link
Contributor

zaggino commented Aug 26, 2016

@dakaraphi in the meantime, you can do this:

FileSystem.prototype._indexFilter = function (path, name) {

        var stats = this.getFileForPath(path)._stat;
        if (stats) {
            ...
        }

This would require probably calling projectRefresh event after it's first loaded because first run of this won't have stats.

I assume that this: https://github.com/dakaraphi/brackets-filter-extension#example-configuration is the configuration required by your project?

@ficristo
Copy link
Collaborator

I think that before we have something in the tree we will have to wait still a lot of time.
I still believe this is not right, but meh. We will survive with it.
@zaggino could you check if this is still valid and land?

@dakaraphi keep in mind that is not a supported behaviour and if this will be in the way we will remove it.

@zaggino zaggino merged commit 75406e8 into adobe:master Dec 14, 2016
@zaggino
Copy link
Contributor

zaggino commented Dec 14, 2016

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants