Ilmar [ilmar] [#821982]

Comment	File	Size	Author
#9	ipgeobase.tar_.gz	3.51 KB	Ilmar
#1	ipgeobase.tar_.gz	3.5 KB	Ilmar

Status	File	Size
new	Module code	3.5 KB

Comment #2

Ilmar commented 8 June 2010 at 18:51

Status:

Postponed (maintainer needs more info)

» Needs review

Log in or register to post comments

Comment #3

avpaderno

he/him

Italian

Brescia, 🇮🇹 🇪🇺

commented 9 June 2010 at 04:31

Issue tags:

+Module review

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.

Log in or register to post comments

Comment #4

Ilmar commented 10 June 2010 at 19:13

Hi! Great, thanks, I'll be waiting for the feedback :)

Log in or register to post comments

Comment #5

Ilmar commented 13 August 2010 at 08:35

Hello! Is there anything I should do to improve the code in order to meet the Drupal standards? Just let me know, I'm still waiting :)

Log in or register to post comments

Comment #6

Ilmar commented 21 August 2010 at 11:05

Anybody? :)

Log in or register to post comments

Comment #7

avpaderno

he/him

Italian

Brescia, 🇮🇹 🇪🇺

commented 11 September 2010 at 16:54

Status:

Needs review

» Needs work

The points reported in this review are not in order or importance / relevance.
Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.

```
package = IpGeoBase
```
The package is used when a module is part of a group of modules; if the package name is not one already used by other modules, it should not be set.
The module doesn't delete the Drupal variables it defines when hook_uninstall() is invoked.

Log in or register to post comments

Comment #8

Ilmar commented 16 September 2010 at 19:01

Thanks for the feedback! I'll address those issues.

Log in or register to post comments

Status	File	Size
new	ipgeobase.tar_.gz	3.51 KB

I'm not sure I have understood why I shouldn't set the package name (although I've removed it anyway :) I've created 2 info files in order to divide the api and block logic. I guess this module's blocks will not be used as frequently as the core API for geolocating so I thought that it might be good idea to let end-user decide which features will be required and do not introduce unneeded hooks into the system. Could you explain this a bit? I have read through the CVS application review, but had not found any explanation there.

Thanks! :)

Log in or register to post comments

Comment #10

avpaderno

he/him

Italian

Brescia, 🇮🇹 🇪🇺

commented 5 October 2010 at 00:11

Status:

Needs review

» Fixed

Thank you for your contribution!
I am going to update your account so you can opt into security advisory coverage now.
These are some recommended readings to help with excellent maintainership:

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, also, for your patience with the review process.
Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

I thank all the dedicated reviewers as well.