Reviews and Mentoring for mpdonadio

We encourage users to post events happening in the community to the community events group on https://www.drupal.org.
You are viewing a wiki page. You are welcome to join the group and then edit it. Be bold!

This page serves mpdonadio's project application reviews and help to project application reviews. This is a part of mentoring to become a code review administrator eventually. Add any advice that you may have through comments.

Reviews. Some projects have multiple reviews; first is linked. I think this is chronological. #11 and on represent reviews done since the call to action.

  1. Token i18n Macrons
  2. Folder Menu
  3. Simple Pin Map
  4. Ubercart mbe4 Mobile Payment Method
  5. Simple Node Archive
  6. jQuery Scroll Follow
  7. Role memory limit
  8. MultiAnalytics
  9. Search Modify
  10. [D7] TAP CMS
  11. [D6][D7] Mobi2Go
  12. [D7] Commerce Cart Stats
  13. [D7] Commerce Payfirma Payment Module
  14. [D7] Trebutra - Trello Bug Tracker
  15. [D7] Commerce Google Tag Manager
  16. [D7] MyCharts module
  17. [D7] Ubercart Legal integration
  18. [D7] Sticky Sharrre Bar
  19. [D7] Context Flag - https://drupal.org/node/2213685#comment-8800299
  20. [D7] Nano Scrollbar - https://drupal.org/node/2220033#comment-8800411
  21. [D7] Video Embed Instagram - https://drupal.org/node/2212035#comment-8800763
  22. [D7] Commerce Moolah - https://drupal.org/node/2212461#comment-8801741
  23. [D7] Social Media Aggregator - https://drupal.org/node/2245645#comment-8801819
  24. Youtube Video Uploader - https://drupal.org/node/1874650#comment-8803161
  25. [D7] The City Group Map - https://drupal.org/node/2219823#comment-8804667
  26. [D7] Webform tracking - https://drupal.org/node/2220111#comment-8805245
  27. [D7] Custom Import - https://drupal.org/node/2222137#comment-8806845
  28. [D7] Age Verification Module - https://drupal.org/node/2013591#comment-8818543
  29. [D7] prettyPhoto Formatter - https://drupal.org/node/2215467#comment-8821327
  30. [D7] disable js - https://drupal.org/node/2223193#comment-8823057
  31. [D7] Semantic UI - https://drupal.org/node/2211003#comment-8823281

Comments

Welcome

klausi's picture

Thanks for starting this, looking forward to your reviews! Feel free to ask questions here or in #drupal-codereview on IRC.

Any feedback as to what I

mpdonadio's picture

Any feedback as to what I have been doing well, and what I could improve on would be appreciated. Thanks.

Looking good

klausi's picture

Your API knowledge is strong, you have a sense for detail and a good understanding of the code you are looking at. It looks like you found XSS problems in some cases, so bonus medal of security honour. Respect!

Project application reviews are basically sanity checks, so while providing all details what can be improved is very valuable we have to consider what issues are really blocking approval. If we are confident that the people basically know what they are doing then we should not hold them back. With the exception of security issues and licensing issues.

I personally set applications to needs work if the API usage is so painfully wrong or the spaghetti code issues just keep piling up. Fortunately people fix all the things from pareview.sh, so at least we don't have to complain about coding standards anymore.

I saw that you never mentioned licensing or project duplication issues in your reviews, maybe there were none. See also the project application checklist: https://drupal.org/node/1587704

Thank you!

Thanks, I really appreciate

mpdonadio's picture

Thanks, I really appreciate it. I'll work on better feedback about why something is a blocker, and be more explicit about licensing and duplication, especially when marking as RBTC.

I think it's time to promote

klausi's picture

I think it's time to promote you to git admin now: https://drupal.org/node/2276911

Thanks

mpdonadio's picture

Thanks. I think I have a pretty good feel for what (codewise) is a blocking issue and what isn't now. The one thing I haven't totally gotten a hang of is when an application should be denied because of duplication (I haven't found any good examples yet). For example, what would the proper response be to these two: https://drupal.org/node/2235863 and https://drupal.org/node/2013591

No application should be

klausi's picture

No application should be denied solely on the grounds of duplication. If people absolutely must duplicate efforts for whatever reason we should let them.

That said, we should really push hard our community spirit of collaboration over competition. That means we ask them to contact maintainers of existing projects first, open issues etc. If the maintainer rejects them, then we can continue the application. I demand that they at least document the other existing similar projects and the differences to them on their project page.

Promotion done,

klausi's picture

Promotion done, congratulations! Feel free to ask me any questions that you might have when using your new powers: https://drupal.org/node/1125818 . You can find me in IRC in #drupal-codereview.