Skip to content
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

Issue #989 - filter handler is inefficient #990

Merged
merged 1 commit into from
Sep 18, 2015

Conversation

esjewett
Copy link
Contributor

Addresses issue #989 by removing the unnecessary and inefficient initial filter(null) and introduces logic for using filterExact in situations where it will work.

@esjewett
Copy link
Contributor Author

esjewett commented Sep 6, 2015

I suspect I screwed up this pull request. Anything I can do to help it be merged? Back out and redo without the generated files?

@gordonwoodhull
Copy link
Contributor

Don't worry about it, I can handle that as long as the changes to the non-generated files are all intentional.

I just was in a rush before vacation and didn't have time to review and see whether more tests are necessary, etc. I'm back next week and hope to merge then.

My turnaround tends to be frustratingly slow because I try to seriously review everything. There was a time when stuff was getting merged in too fast and it led to a lot of bugs, some still not fixed. :-(

@esjewett
Copy link
Contributor Author

esjewett commented Sep 7, 2015

Sorry, didn't realize you were on vacation! Enjoy :-) And +1 for reviewing thoroughly. I'm still not completely clear on all the different filter structures that various dc.js components use, so it would be good if you can take a second look and make sure this won't break anything that's not in the test suite.

@gordonwoodhull gordonwoodhull modified the milestone: v2.0 Sep 14, 2015
@gordonwoodhull
Copy link
Contributor

Wow, this "feels" way, way faster on the main demo page! (Admittedly, I haven't benchmarked.)

Next I'll see if I can correctly detect dc.filters.RangedFilter

gordonwoodhull added a commit that referenced this pull request Sep 18, 2015
for #990
@esjewett, I think this is a slightly safer way to check for a custom
filter function such as those in dc.filters
@gordonwoodhull gordonwoodhull merged commit d7ffd14 into dc-js:master Sep 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants