With this module you are able to create a content type "GCCode" with an official gc-code from geocaching.com as title. By importing gpx-files, attributes from gc.com will be shown in the node. Alternatively you can launch a download of informations by clicking on a button in the node. For this implementation the ruby-script geotoad is used in background. The content typ itself offers space to enter your status on the geocache and notices. So this module is about creating your own personal geocaches database.

GIT-Repository:
http://drupalcode.org/sandbox/ab_connor/1436376.git

Version:
Drupal 6.x

Comments

rodrigo panchiniak fernandes’s picture

rodrigo panchiniak fernandes commented
Status: Needs review » Needs work

Hi ab_connor,

Please provide a link to your sandbox project.
Please provide in the issue its git clone command.
Follow this guide http://git.drupal.org/sandbox/ab_connor/1436376.git to correct style and other errors in your code.

Best regards!

ab_connor’s picture

ab_connor commented

As I posted above, the link to my sandbox project is
http://drupalcode.org/sandbox/ab_connor/1436376.git

The clone command is
git clone --branch master ab_connor@git.drupal.org:sandbox/ab_connor/1436376.git gccode

The link for the guide, you posted, isn't correct. Did you look over the code? I spend a lot of time to follow the guidelines. Please be more specific.

Greetings,
Andreas

rodrigo panchiniak fernandes’s picture

rodrigo panchiniak fernandes commented

Sorry for that,

This is the link: http://ventral.org/pareview/httpgitdrupalorgsandboxabconnor1436376git

ab_connor’s picture

ab_connor commented
Status: Needs work » Needs review

Thanks for the link.

I corrected most of the style errors, but I really worry about the number of spaces your script expects. I used Geany to write the code and I haven't found a setting to get the expected number of spaces at all. It seems that Geany always adds two spaces more in total than expected. I'm not sure, I'm so patient to manually correct more than a thousand lines of code. Should the project fail on this? Or could we turn a blind eye and carry on?

Also there are some messages, I wasn't able to interpret.

To say it in a summary: I did my very best.

Greetings,
Andreas

patrickd’s picture

patrickd commented
Status: Needs review » Needs work

Your still working in the master branch and still got a wrong prefixed function in your .install file. Please fix these major issues before switching back to needs review.

Please provide a english readme.

You should search for the indentation setting in your code editor, 2 spaces are best practice

/**
 * Implements hook_menu().
 */
function gccode_menu() {
    // The administration menu.
    $items['admin/settings/gccode'] = array(
        'title' => 'GC-Code',

=>

/**
 * Implements hook_menu().
 */
function gccode_menu() {
  // The administration menu.
  $items['admin/settings/gccode'] = array(
    'title' => 'GC-Code',

If you got problems, please ask more precise what you did not understand.

ab_connor’s picture

ab_connor commented
Status: Needs work » Needs review

All issues fixed now.

Greetings,
Andreas

patrickd’s picture

patrickd commented
Assigned: ab_connor » Unassigned

great,
please don't assign the application to your self, only the current reviewer should do this

novalnet’s picture

novalnet commented

Hi,

Manual Review :

1.There is a space in ' (' in line 137 of gccode.module.Does it needed for any future purpose?
2.Use t() in drupal et title function.It seems it contains dynamic values, so it would be better if you use placeholders also.
3.Use drupal string functions instead of normal string functions in line 495 of gccode.module.
ex : substr() should be drupal_substr(). Also check it in all other places.
4. Please use l() to create link markup in line 559 of gccode.module.
5.It seems you are missing @ inside array in drupal_set_message(t("Geotoad result could not be imported. Please inspect @x .", array('x' => $res_file)), 'error');
It should be drupal_set_message(t("Geotoad result could not be imported. Please inspect @x .", array('@x' => $res_file)), 'error');

Thanks,

ab_connor’s picture

ab_connor commented

Hi Novalnet,

thanks for this review.

1.There is a space in ' (' in line 137 of gccode.module.Does it needed for any future purpose?

If a matching entry exists in the database table "gccode_info", the title is replaced by the headline of the cache and completed with the GC-Code in brackets. Did I get your question right?

2.Use t() in drupal et title function.It seems it contains dynamic values, so it would be better if you use placeholders also.

Translation doesn't make sense, because the dynamic value is an fixed phrase that shouldn't be altered. But I inserted a "check_plain", if there are any HTML-Tags in it.

3.Use drupal string functions instead of normal string functions in line 495 of gccode.module.
ex : substr() should be drupal_substr(). Also check it in all other places.

Hmm. For what? Drupal documentation itself says:

Note that for cutting off a string at a known character/substring location, the usage of PHP's normal strpos/substr is safe and much faster.

4. Please use l() to create link markup in line 559 of gccode.module.

Thanks and done.

5.It seems you are missing @ inside array in drupal_set_message(t("Geotoad result could not be imported. Please inspect @x .", array('x' => $res_file)), 'error');
It should be drupal_set_message(t("Geotoad result could not be imported. Please inspect @x .", array('@x' => $res_file)), 'error');

You're right. Done.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Priority: Normal » Critical
thursday_bw’s picture

thursday_bw commented

README.txt

1. Has a lot of trailing whitespaces on lines of text.

2. Although there is no standard consensus for README.txt file style. This page offers a few suggestions: http://drupal.org/node/447604

3. Please check spelling and grammar in the README file.

* "This modul can help..." module
* "Solutions are not to blat out." is 'blat' correct, this is an unknown word
* "Please act responsable." Please act responsibly.
There are other grammar and spelling issues, I won't list them all. Please proof read.

gccode.install

4. Missing a file doc comment from you .install file

5. There is no need to set the variables in gccode_install(). As the variable_get should be used with default
values in code anyway.

6. In your description in the schema, the word 'informations' should not be plural.

gccode.info

7. gccode.info appears to be a binary file and I cannot view it's contents.

gccode.module

8. The array returned in gccode_perm() should probably have each item on it's own line, instead of just two lines.

9. Please add plenty more in-line comments to explain what your thinking is, especially in the following functions:
gccode_access
gccode_information_form
gccode_search_form
gccode_validate
gccode_block

among others.

10. function gccode_help()
Please consider placing the contents of the $output variable into a themable template.

thursday_bw’s picture

thursday_bw commented
Status: Needs review » Needs work
thursday_bw’s picture

thursday_bw commented
Priority: Critical » Normal
ab_connor’s picture

ab_connor commented
Status: Needs work » Needs review

Hello bevanw,

at first I would like to thank you for the review.
Now I answer your points:

1. Has a lot of trailing whitespaces on lines of text.

Fixed.

2. Although there is no standard consensus for README.txt file style. This page offers a few suggestions: http://drupal.org/node/447604

Well, I found no serious inconsistency with this text.

3. Please check spelling and grammar in the README file.

* "This modul can help..." module
* "Solutions are not to blat out." is 'blat' correct, this is an unknown word
* "Please act responsable." Please act responsibly.
There are other grammar and spelling issues, I won't list them all. Please proof read.

Sorry for my english. I did my very best. I fixed the mistakes, you listed. But I didn't find any more. Please feel free to make some more suggestions.

4. Missing a file doc comment from you .install file

Fixed.

5. There is no need to set the variables in gccode_install(). As the variable_get should be used with default
values in code anyway.

The variables set there, are owned by foreign modules and change some standard options. I think it's the best place to do it, isn't it?

6. In your description in the schema, the word 'informations' should not be plural.

Fixed.

7. gccode.info appears to be a binary file and I cannot view it's contents.

Strange, isn't it? Fixed.

8. The array returned in gccode_perm() should probably have each item on it's own line, instead of just two lines.

Fixed.

9. Please add plenty more in-line comments to explain what your thinking is, especially in the following functions:
gccode_access
gccode_information_form
gccode_search_form
gccode_validate
gccode_block

among others.

I hope, it's more satisfactory now.

10. function gccode_help()
Please consider placing the contents of the $output variable into a themable template.

I didn't get what you mean. I took the related function in comment.module as pattern and it's nearly the same. So what do you want me exactly to do?

Greetings,
Andreas

vensires’s picture

vensires
Location Greece
commented
Status: Needs review » Needs work

About function gccode_help(), I have seen similar "hook_help()"s within other full projects, so it's not so unfamiliar that it's so long. bevanw has a point though. You could use hook_theme() to theme the help contents with a proper .tpl file but what would be best, is to use the possibilities the advanced_help module opens for you. I'm not familiar with advanced_help module either, but I suppose that a great help would be if you checked the Views module (a bunch of documentation is found in .html files through advanced help module).

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application.