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

Don't add empty filter sets to the file filter dropdown list.#7975

Merged
bchintx merged 1 commit intomasterfrom
rlim/empty-filter-set
May 29, 2014
Merged

Don't add empty filter sets to the file filter dropdown list.#7975
bchintx merged 1 commit intomasterfrom
rlim/empty-filter-set

Conversation

@RaymondLim
Copy link
Copy Markdown
Contributor

This fixes #7973 and #7974.

@TomMalbran
Copy link
Copy Markdown
Contributor

@RaymondLim Will a validator for the file filters preference help with this issues?

@RaymondLim
Copy link
Copy Markdown
Contributor Author

Not sure a validator may help since current filters preference is an object and not a simple primitive type or just an array. And I think creating a validator may need more code than what we have right now and may not really simplify the code.

@bchintx bchintx self-assigned this May 29, 2014
Comment thread src/search/FileFilters.js
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@RaymondLim
Instead of just dropping patterns that aren't defined in the Preferences file as an array, could we allow them by just returning the single pattern?

For example, instead of returning an empty string here, could we just return filter; in the line above? That would allow a user to specify a single pattern (ie. without the array notation) in the Preferences file like this:
"fileFilters": [ { "name": "JS files", "patterns": "*.js" } ]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@bchintx I think we should enforce the consistency of our preferences format and data type. Otherwise, we may end up with complicated code that handles multiple data types. For example, how about multiple filter strings without the []? I know that multiple strings will break the JSON file, but it's possible that someone may mistakenly create a filter set with no enclosing brackets. So instead of handling one extra case, I'd rather enforce the correct data format and data type.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@RaymondLim Ok, sounds fine.

@bchintx
Copy link
Copy Markdown
Contributor

bchintx commented May 29, 2014

@RaymondLim Done w/ review. Looks great, and I've verified the change fixes both #7973 and #7974. I've just one request in-line above.

@bchintx
Copy link
Copy Markdown
Contributor

bchintx commented May 29, 2014

Changes look good. Merging...

bchintx added a commit that referenced this pull request May 29, 2014
Don't add empty filter sets to the file filter dropdown list.
@bchintx bchintx merged commit e71e09a into master May 29, 2014
@bchintx bchintx deleted the rlim/empty-filter-set branch May 29, 2014 20:59
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.

Uncaught TypeError: Object *. has no method 'join' in console if you change file filter in preference file.

3 participants