Posted by kalman.hosszu on June 28, 2010 at 7:11pm
| Project: | Drupal.org CVS applications |
| Component: | Miscellaneous |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | module review |
Issue Summary
CVS edit link for hosszu.kalman
I am Kálmán Hosszu, 25 years old hungarian developer. I write Drupal modules since version 4.7.I'm working as the head of Drupal Group for a company called Factory Creative Studio (http://factory.co.hu/English/). I am also an active member of the hungarian Drupal community, providing help and support for others via email, community forum, irc and skype. Other information for developers can be found on my personal blog.
The first module I would like to share with the Drupal community helps developers to create new element types like number, email, url or slider.
Comments
#1
I attached the module "custom_elements" to this comment.
Thank you for your review!
#2
Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review your code, pointing out what needs to be changed.
As per requirements, the motivation message should be expanded to include a description of the features implemented by the proposed module / theme. For modules, it should also include a comparision with the existing projects; for themes, it should include a screenshot of the theme and, when possible, a link to a site using the theme.
#3
#4
More information about the module:
The module is designed to assist developers by creating frequently used form element types.
These form types often come forth during development stages, and the validation and description should always be written.
The module creates 4 element types:
It carries out the validation of these element types, so developers don't have to worry about this.
The number type has two settings:
During validation, the module watches the values, if they comply with the specified parameters. If the values are filled out, they also get into the description section, so the user can be informed.
The slider type works only if the jquery_ui module is enabled. Then the developer has the following settings:
Thank you for your review.
#5
#6
What is the next step? Should I do anything?
#7
Hi Kálmán,
Firstly, thanks for contributing - it's great to see people willing to share their work :-)
The CVS application guide describes how the Drupal community review CVS applications, and what's considered when reviewing a submission.
I've run your custom_elements module through coder module which reports 6 normal warnings and 25 minor warnings. The warnings are about coding style - having a consistent coding style makes it easier for the community to share, understand and test code, so we'd prefer to see all contributions at least passing the coder module's tests.
Your code shows a great understanding of Drupal APIs, Formapi, and the separation of presentation with the theme layer.
Personally, I prefer to see theme functions moved out of module files into separate theme-include files (because module files are loaded for every full-bootstrap request, whilst theme-files are loaded on-demand: therefore separate files will generally reduce memory-usage), but this is a matter of personal opinion.
You may find it helpful to have native English speakers review the text, for example "jQuery UI module is not exist." should probably say "jQuery UI module is not available." or "jQuery UI module does not exist." (although I'll confess to typos and language errors slipping through in my modules, despite copious review!)
If a module is part of a collection of modules - for example, Views, Views UI, and Views-bulk export all belong together - then use the package parameter in the .info file to group these modules together. If it's a stand-alone module, then the package parameter can be left out.
I would add something to the module's description (in the info file) to describe this as an API module (in the same way as http://drupal.org/project/votingapi), so that people who aren't developers don't add the module in expectation of some sort of interface.
The module does appear to duplicate features provided by other modules, for example:
Modules which duplicate features of other modules are generally discouraged, because it becomes harder for members of the community to select an appropriate module for a task. There's a similar module review group which helps identify module-duplication and discuss whether a module is providing a unique benefit, or whether to encourage a developer to collaborate with another module which is working towards the same goal. I'd encourage you to collaborate with those modules - especially CCK - and merge the features you feel are unique to your module with these others.
I hope you find this feedback useful, and thanks again for getting involved!
#8
I am changing the status as per previous comment.
#9
Thank you very much for reviewing the code! I will change the mentioned things in a few days.
But I think you misunderstand the task of the module. This module is a helper module, it's point is that developers don't have to install other modules unnecessarily .
For instance, if a module has a settings page, where email addresses, URLs, and some numbers should be given, then just to avoid having to write the validation functions, we don't want to install the CCK add-ons for those field types, since we don't want to create a field on a node.
This small module is intended for developers who don't want to create these types every time, nor write the validation functions and messages, but they want to use it such as using the 'date' type, for example.
So it is understandable?
#10
According to my previous comment I fixed and attached the module.
Thank you again for your review!
#11
@manarth: any further comments? Thanks!
#12
Hi Kálmán,
The code is readable, documented, and uses the Drupal APIs - all in all, exactly what we like, and a welcome contribution :-)
I've just been pointed towards elements (a module which I'd not come across until today!) which has similar project goals to yours: a repository of advanced FAPI elements. The elements project already provides number, email, and url elements, but no slider-control, so I would encourage you to submit your slider-control as a patch - the module maintainers are actively inviting new elements as patches.
-- manarth
#13
#14
Why has this application been marked as won't fix?
#15
I agree with manarth that the proposed module duplicates the work done in a existing project, which seems to duplicate the work done by another existing project but that is without any official releases.
Are there any reasons you could not collaborate with the maintainers of http://drupal.org/project/elements, or http://drupal.org/project/extended_html?
#16
My bad - I probably chose the wrong status. I didn't want to mark as needs work, because there's nothing wrong with the module code that should be addressed. Collaboration with the other module(s) would be my first choice of approach.
#17
@manarth: As reported in CVS Applications issue queue workflows, if there's a problem or clarification is needed after review, the question should be asked and the reviewer should change the status to needs work.
Settings a CVS application to won't fix should be done by a CVS administrator, as there is still a step to be done (to decline the application, which is done by a CVS administrator with the link CVS edit link for hosszu.kalman); if the application is not declined, the applicant cannot apply for a CVS application anymore.
Even in the case the module duplicates the work done by an existing project, we always want to hear from who is applying the reason why he thinks his module is worth to be placed in Drupal.org repository; that doesn't mean the module will be accepted for the CVS application, but it also doesn't mean the proposed module is always declined.
#18
@kiamlaluno - apologies, I clearly chose the incorrect status. Thanks for clarifying and pointing me to the workflow page.
#19
I requested to be a co-maintainer in Soundcloud filter module, and the module's maintainer accept my request. Please grant cvs access for co-maintainership of this module. You can check the discussion at: http://drupal.org/node/944276
#20
#21
Automatically closed -- issue fixed for 2 weeks with no activity.