Provides numeric range field. Inspired by core Number module. Module code inherited from the core.

Project page: http://drupal.org/sandbox/Taran2L/1848632

Git repository: git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/Taran2L/1848632.git range

Version: Drupal 7.x

Comments

nesta_’s picture

Status: Needs review » Needs work

README.txt is missing, see the guidelines for in-project documentation.

Taran2L’s picture

Status: Needs work » Needs review

Added README.txt.

facine’s picture

Status: Needs review » Needs work

Hi!

Thanks for submitting a project application for review.

The PAReview tool identified a few issues
http://ventral.org/pareview/httpgitdrupalorgsandboxtaran2l1848632git

Manual review:
I have not found any other errors that need fixed except those mentioned in the PAReview tool.

Can you please check/fix the above issues? Once done change the status to "needs review"

Taran2L’s picture

Fixed all codestyle related issues, except:

16 | ERROR | If the line declaring an array spans longer than 80 characters,
 | | each element should be broken into its own line
 23 | ERROR | If the line declaring an array spans longer than 80 characters,
 | | each element should be broken into its own line
 24 | ERROR | If the line declaring an array spans longer than 80 characters,
 | | each element should be broken into its own line
 32 | ERROR | If the line declaring an array spans longer than 80 characters,
 | | each element should be broken into its own line

That code was copied from core number.module file.

Taran2L’s picture

Status: Needs work » Needs review
jhaskins’s picture

  1. Your project page is a bit sparse. Check out Tips for a great project page
  2. Your module is called "range" but resides in the "range_field" folder. A little unusual but I'm not sure if there's actually anything that says you can't do things that way.

Regarding #4 - I'm not sure what the protocol for that is. I can understand where you're coming from but sticklers for coding standards may insist on them being fixed. Perhaps if you fix it in your module you can also offer up a core patch to bring it inline with the standards.

Aside from that, I don't see any issues with the code & the module works as expected without any errors.

dasRicardo’s picture

Hello,

first sorry for my bad english.

I tested the module also, and think everything is fine. No errors and it works perfect.
I mean it's a little bit confusing that your module name is another like the folder name.

Taran2L’s picture

jhaskins and das_ricardo, thank you for your reviews.

Folder name is different because I made a mistake in the clone line. Fixed.

Project page has been updated. I've included more info about modules features.

Regarding #1848868-4: Range field, I do not know the procedure either, but I think if this code exists in core, it can exist in contribute module.

Thanks again for your time!

DYdave’s picture

Status: Needs review » Needs work

Hi Taran2l,

Thanks very much for your reply and explanations.

Your last post attracted my attention and in particular:

Regarding #1848868-4: Range field, I do not know the procedure either, but I think if this code exists in core, it can exist in contribute module.

Unfortunately, I think the PAReview is telling the truth and there are coding standards errors in the core number module from which you copied the code. But this shouldn't mean that Range field should have the same errors, should it?

Besides, I would tend to agree with the previous comment #6:

Perhaps if you fix it in your module you can also offer up a core patch to bring it inline with the standards.

which is I guess would be the good standard attitude to adopt in such situations.

Therefore, I tried to investigate a bit more this issue and posted: #1867468: PAReview of Drupal core project?
where klausi kindly and so promptly guided me to #1518116: [meta] Make Core pass Coder Review, then #1533232: Make field module pass Coder Review where I finally found the supposed patch mentioned at #6 about core coding standards for the Field module (In particular: number.module):
See: #1533232-2: Make field module pass Coder Review, with patch: drupal-code_cleanup-1533232-3.patch.

I have checked and all the fixes in this patch, applied to the file number.module would apply to drupal-7.x, number.module file as well (manually, not directly, I guess).
I recommend you check this patch carefully, to double check this information and in particular I invite you to install Dreditor which could greatly help you in your review of this very long patch.

Here is an extract that concerns Range field and I would like to attract your attention on how the following errors, for example:

16 | ERROR | If the line declaring an array spans longer than 80 characters,
 | | each element should be broken into its own line
23 | ERROR | If the line declaring an array spans longer than 80 characters,
 | | each element should be broken into its own line

were fixed:

diff --git a/core/modules/field/modules/number/number.module b/core/modules/field/modules/number/number.module
index 9985984..e6823f2 100644
--- a/core/modules/field/modules/number/number.module
+++ b/core/modules/field/modules/number/number.moduleundefined
@@ -26,7 +26,12 @@ function number_field_info() {
     'number_integer' => array(
       'label' => t('Integer'),
       'description' => t('This field stores a number in the database as an integer.'),
-      'instance_settings' => array('min' => '', 'max' => '', 'prefix' => '', 'suffix' => ''),
+      'instance_settings' => array(
+        'min' => '',
+        'max' => '',
+        'prefix' => '',
+        'suffix' => '',
+      ),
       'default_widget' => 'number',
       'default_formatter' => 'number_integer',
     ),
@@ -34,14 +39,24 @@ function number_field_info() {
       'label' => t('Decimal'),
       'description' => t('This field stores a number in the database in a fixed decimal format.'),
       'settings' => array('precision' => 10, 'scale' => 2),
-      'instance_settings' => array('min' => '', 'max' => '', 'prefix' => '', 'suffix' => ''),
+      'instance_settings' => array(
+        'min' => '',
+        'max' => '',
+        'prefix' => '',
+        'suffix' => '',
+      ),
       'default_widget' => 'number',
       'default_formatter' => 'number_decimal',
     ),
     [..... MORE IN THE PATCH .....]

 

To cut it short:

  1. Indeed, fixing coding standards is a work in progress in the drupal core: #1518116: [meta] Make Core pass Coder Review
  2. If you really wanted to copy the correct code, you would be copying it from the patch and not from the current core module's version.
  3. In turn, these issues wouldn't exist in Range field's automated project review.

Now, the point of this long explanation is to try to convince you to fix these last errors and move forward this project application.
At this point, I would recommend you take a careful look at the previous comments, fix the last reported coding standard issues, based on the code in the suggested patch.

I think if you have written this module and got as far already in the validation process and development of it, you should certainly be able to take down this last part coding standard errors, which really doesn't need that much work or time (It should be very quick, much quicker than writing this comment). It is important that you understand coding standards are essential for testing, code validation, maintenance and review by other developers in the community that would like to involve in future developments for Range field.

I strongly advise that you try spending some more time investigating further developers modules for coding standards such as the Coder module. You would be able to see that a lot of Drupal Core modules actually prompt coding standards validation errors (but I guess it's a work in progress.... and if errors aren't tracked yet, a new issue should be created).

If you have tested the module again and everything is fine, then I would recommend you change the status to needs review and work on reviewing others' applications to get a #1410826: [META] Review bonus, since it seems you didn't get any so far.
That would allow adding the tag PAReview: review bonus to your application, which would get you on the fast lane for validation until you get the attention of official reviewers.

Feel free to let me know if anything in this detailed review would be unclear or if you would have any questions on any aspects of this comment, I would surely be glad to provide more clarifications.

Well done! Nice work, and I sincerely hope these comments help you improving this very nice module.
Thanks again for your comments and feedbacks.
Cheers!

Taran2L’s picture

Status: Needs work » Needs review

Hi DYdave,

Thanks for such detailed comment. Very appreciated your time.

Last codestyle related issues have been fixed.

anwar_max’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +PAReview: admin mentoring

Hi Taran2l,

Everything is looking good to me unable to find any issue.

stBorchert’s picture

It seems that Range provides very similar functionality. Could you describe how your module differs?
We prefer collaboration over competition, therefore we want to prevent having duplicating modules on drupal.org. If the differences between these modules are not too fundamental for patching the existing one, we would love to see you joining forces and concentrate all power on enhancing one module.
If the existing module is abandoned, please think about taking it over instead of creating a new one.

Leaving this on RTBC for now ...

Taran2L’s picture

Hi stBorchert,

Range module you are referencing to exists for D6 only. My module adds range field to the D7. That's the main difference.

I'm ok to merge D7 version with existing project.

klausi’s picture

Status: Reviewed & tested by the community » Postponed (maintainer needs more info)

Great! Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the range issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

Shevchuk’s picture

Any news on this?

Taran2L’s picture

I've contacted D6 module maintainer but didn't get any answer: #1877762: RANGE support request.

Moved issue to "webmasters queue" in order to become project maintainer.

Taran2L’s picture

Status: Fixed » Postponed (maintainer needs more info)

Issue in the "webmasters queue" has been resolved and now I'm the maintainer of the Range field. Will merge my D7 version into it ASAP.
I believe this issue could be closed.

klausi’s picture

Status: Postponed (maintainer needs more info) » Fixed

Yep, thanks!

Taran2L’s picture

Status: Postponed (maintainer needs more info) » Fixed
kenorb’s picture

DYdave’s picture

Hi Taran2l,

Thank you very much for your time, efforts and we're glad to see the Range module now offers a Drupal 7 version.
On top of that, Drupal 6 users have gained a new maintainer that seems to be more proactively engaged in the module at this time (#1877762: RANGE support request).

We will certainly let you know if we find any new issues, have more recommendations or ideas by creating tickets in module's tracker.

Thanks again very much for your contributions and involvement.
Cheers!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Changed folder name in the clone line.

Taran2L’s picture

Seems like I'm unable to create full projects, because my module hasn't been promoted to a Full project, but I became maintainer of a another module.

Is this a reason ? Or am I missing something ?

klausi’s picture

Yes, you have not been granted the git vetted user role yet. Please create a new project application with the sandbox you want to promote.

Taran2L’s picture

Issue summary: View changes

Hi @klausi,

Sorry for bothering you again :)

I have added some tests to the module, but can't enable them for the project. Probably due to missing permissions, i.e. I don't see the "Enable automated testing" checkbox under "Issues" tab on the project edit form.

I don't have any other project I would like to promote atm. Could I be given git vetted user role? Range module has over 1000 installs atm. I do believe it's fair enough.

Thanks!