Debut Location

Hi pmackay,

• Remove "project" from the info file, it will be added by drupal.org packaging automatically.
• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

Also
FILE: ...ew/sites/all/modules/pareview_temp/test_candidate/debut_location.ie.css
--------------------------------------------------------------------------------
FOUND 3 ERROR(S) AFFECTING 3 LINE(S)
--------------------------------------------------------------------------------
3 | ERROR | Multiple CSS properties should be listed in alphabetical order
4 | ERROR | Multiple CSS properties should be listed in alphabetical order
5 | ERROR | Multiple CSS properties should be listed in alphabetical order
--------------------------------------------------------------------------------

FILE: ...ew/sites/all/modules/pareview_temp/test_candidate/debut_location.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
44 | ERROR | If the line declaring an array spans longer than 80 characters,
| | each element should be broken into its own line
48 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

Automated review at
http://ventral.org/pareview/httpgitdrupalorgsandboxpmackay1571202git

Need exact documentation how people will get Yahoo PlaceFinder API Key and Yandex Maps API Key. You had specified the link but I'm stuck to get that and also follow coding standard : http://drupal.org/coding-standards/#linelength

no major issues pointed out -> don't set needs work

The documentation needs to be improved drastically for this module, so I'm moving this to "needs work".

Could you provide some extra information on the specific functionality provided by this feature on the project page and in the README.txt file. "Geolocation functionality" is an extremely ambiguous concept and currently there's no way of knowing what the module does without downloading and installing it.

A summary outline of what use-cases you're addressing here and what can be achieved "out of the box" with this module is completely absent.

Some more information on how this deals with different use-cases to similar mapping Features like http://drupal.org/project/ol_locator would be really useful for users that are comparing different options - mapping is already complicated enough for people new to the process because a small change in the project requirements can completely change how you want to implement your maps.

The nature of Features like this is that you end up with a lot of dependencies quickly and I see you even have a patch in there as well. This is not a problem in itself but you need to make it very clear what your dependencies are and what your patches are achieving in both the README.txt file and your project description.

On a more minor note, you might want to consider putting your .js and .css and image files in subdirectories of your module for improved organisation. You could also move the custom code you've written in your main .module file into an included file within a subdirectory too. That way it is clearer to your end-users what is automatically generated by Features and what you've written yourself.

@thedavidmeister and others: thanks for the reviews!

I've been collaborating in building this feature and will remain a co-maintainer to provide mentorship and guidance where needed.

While I've added some custom code, I'm working hard to contribute any significant enhancements back into the original modules, e.g., #1627940: Add optional geocoding to map input widget and #1673562: Automatically select children in nested checkboxes option.

Added documentation to the project page, http://drupal.org/sandbox/pmackay/1571202, including:

• Specific functionality provided.
• Summary of use cases.
• Similar modules for comparison.
• List of dependencies.

As soon as we have a stable release, we'll be adding this to the Debut apps server, so it will be a lot easier for site admins to add.

Need exact documentation how people will get Yahoo PlaceFinder API Key and Yandex Maps API Key. You had specified the link but I'm stuck to get that

This question would be related to one of the modules we've used (Geocoder) rather than this feature module directly.

you might want to consider putting your .js and .css and image files in subdirectories of your module for improved organisation

There doesn't seem to be a clear standard here. I tend to follow core, which puts .js and .css directly in the module directory (e.g., user module).

While code can be pulled into include files, I tend to do this only when it increases performance or addresses a clarity concern. Here, it's common practice to put regular module code in a feature's .module file.

Sure, I suppose one of the problems I was trying to address is that the process of reviewing modules in this issue queue is as much about the module as it is the writer of the module. That writeup on the project page looks much better :)

Once your module is approved you will be granted full git access so in reviewing the module through this process the Drupal community is trying to enforce a basic entry-level standard skill level for the programmers allowed to publish directly back to the community.

I'd like to point out that posting a Feature as your first module is an interesting situation as we're all aware that 80-99% of the code in what is being reviewed here was generated by an automated process. This could make it harder to approve the module even if the code is perfect because there may not be enough manual code in there, or it might be so difficult to differentiate the manual code from the automated stuff that the application is rejected or stalled longer than you might like.

What I was getting at with the suggestion to split out includes is that a Feature differs from a core module in a few key ways. One is that the code maintained by the Features module will be overwritten every time you run "$ drush fu" or recreate it manually and will evolve over time as the Features API and the API of modules that implement Features change. This adds an extra layer of complexity for other users wanting to provide patches for you as they may be experienced enough with php to write a fix, but not immediately identify what will be overwritten periodically and where their patches will "stick". I thought making this separation explicit might help make the module more maintainable over the longer term and also make it easier to review. That's just my opinion though...

Posting patches for other contrib modules is indeed the "right" thing to be doing, no argument here :)

Also, don't forget to set your issue back to "needs review" if you've made changes and want more input from the community.

Thanks again for your review time!

A bit of background, as this is sort of an unusual situation.

I'm a longtime Drupal core and contrib developer (see my profile) and the technical lead of the Open Outreach distribution and accompanying Debut app set.

pmackay has been contributing some work to Open Outreach and Debut, including an initial draft of the Debut Location feature. Working on his start, I've done most of the development (and all of the custom coding).

Were this my own project, I would promote it to a full project, alongside the dozen or so other Debut features I maintain. But since Paul posted it, I don't have that access.

Paul contributed a previous project, Debut Newsletter. After a fair bit of community review it was approved in this issue: #1340278: Debut Newsletter. But because it didn't contain custom code, Paul didn't get project maintainer status.

Debut Location has benefited from community review and I'm very happy to have Paul contributing and serving as primary maintainer. But going forward I don't know if this is the best use of review resources. It also complicates my work on my distribution, as I have to wait on an approval process before I can add a new app that I've been building. The process is further complicated by these being feature modules (which follow their own, somewhat arcane, logic) and, on top of that, ones written to a specific spec (the Debut features specification), so that for consistency certain changes made here, like changing file directories, would have to be made in a dozen or so other modules.

I suggest that, for this project and going forward, we aim for an expedited review process. As one of the more experienced Drupal developers, I will be comaintaining and both provide mentorship and help ensure quality. I can't exactly mark this issue RTBC as it's to a significant degree my own work ;) But so far as I can tell, there are no significant outstanding issues and this project is ready to go.

If there are more issues, I can commit to working with Paul to get them quickly fixed up.

Sound good?

ok, so the summary is pmackay isn't applying for git access because nedjo wrote all the custom code and is already a contributor, but pmackay wants this Feature pushed through anyway so everyone can generally benefit since the codebase is backed by nedjo?

I understand totally what you're saying. I've been working with Drupal for about 4 years and I'm just putting my first contrib module up for review now to try and get it promoted to a full project. I don't really have an answer to your situation. If you're saying you find it difficult to consistently write good quality code quickly because you're still in the process of gaining that experience that's kind of a good reason not to grant git access just yet :P

The whole Features situation is definitely a weird one, and something that was possibly not anticipated when this issue queue was first setup. Unfortunately I still think that no matter how awesome the module itself is, we shouldn't be giving pmackay full git access if there is minimal custom code to review in the module he's using for his application and a good portion of what is there was written by someone else :(

Nedjo is listed as the maintainer of the debut project, so he should be able to accept and promote any sandbox code you have without much more formality than opening a ticket on the debut project. Theoretically that process should really not be influenced too much by whether Nedjo has a personal use for the Feature you're contributing or not, as long as it provides good value for the community at large.

It's been a few days since my last post. What ended up happening here? did Nedjo manage to get a copy of this Debut module under his name?

I can't myself promote this to a full project (without signing it over to myself first).

All identified issues with the project have been addressed, so it looks like, as with Debut Newsletter, see #1340278-14: Debut Newsletter, the project should be promoted but pmackay not yet assigned full git access.

Your project page contains very detailed informations, unfortunately your readme is not that detailed. It's always good to keep project page and readme in sync.

Nothing serious here, also good to see that nedjo is helping here so if have no further concerns.

Thanks for your contribution, pmackay!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, 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.

Thanks to the dedicated reviewer(s) as well.