Jump to:
| Project: | Views Filter Pack |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | peter-boeren |
| Status: | closed (fixed) |
Issue Summary
While working on http://drupal.org/node/516242#comment-1825858, I wanted to peek at Views Filter Pack's code to hopefully gain some insight in the barely documented Views APIs. I shouldn't have, because:
- The Drupal coding standards are not respected.
- Your Doxygen isn't properly capitalized.
- Many, many typos.
- Seemingly random line wrapping: sometimes at 60, sometimes at 70. Whereas just about everybody has settled on 80(-ish).
- Your overrides of Views handlers are overrides in their worst form: copy/paste + a couple of *undocumented* changes. How is anybody every supposed to make sense of this? I'm talking about vfp_handler_filter_in_operator, I didn't even look at the others.
Whenever you override things, you should very clearly document *what* exactly you are overriding, *how*, and *why* (what does it achieve?). How else is it possible for others to contribute patches?
(I don't mean to come over harsh and annoying, but the errors are so blatant.)
Comments
#1
Hi Wim,
thank you for an early code review. I'm working according to a method described in this article. At this moment I'm at the first stage 'make it work'. In my opinion this stage is finished when it works with CCK. After that I'll start re-factoring.
I don't like the overrides of views handlers either, but with my current knowledge of views, I don't see another option. It's a lot of code while I would like only to alter one or two functions within a handler. I'm open to better solutions. The situation is as following:
1) with hook_views_data views, or any other module, defines fields and sets the handlers for the filters.
2) with hook_views_handlers() the relation between the handler and its parent is being defined. And this is also done for the fields views supports by default. (I haven't found an alteration hook for this part in views.) The parent-child relation is needed in views for extending the class of the handler (class child extends parent {).
3) with hook_views_data_alter I replace all handlers that have functions that only work with array's (select-boxes) in building the query clause. Because when using a radio button the value of a exposed field is a string.
I don't think your critics are harsh but they are constructive. For example I didn't know there was a standard for line wrapping. I hope when I do a code clean up that I can ask you to review it again.
#2
The feedback is processed in the code. The code is on CVS and will be available after the next run of the packaging script.
- The Drupal coding standards are not respected => The module has been checked by coder on minor-mode. In this mode the most feedback was given. The feedback has been processed.
- Doxygen => Every function has now Doxygen. And a lot of comments has been added to the code to make the code more readable.
- Typo's => Also corrected a lot of them. If you find some more, please let me know.
- Linewrapping => Reviewed all my existing comments and linewrappings. These are now all 80(-ish).
- Override of handlers => I'm afraid this will be by design unless the views module itself is altered.
So everyone who wants to review the code is welcome.
#3