Services Menu [#1212746]

Comment	File	Size	Author
#15	non_existing_menu_rest.png	221.39 KB	Robin van Emden
#15	main_menu_rest.png	231.02 KB	Robin van Emden
#15	amfserver_trace.txt	1.11 KB	Robin van Emden

Comment #1

ccardea CreditAttribution: ccardea commented 12 July 2011 at 16:57

I just took a quick look at your project.

.inc file
files[] is for files that contain class or interface definitions that get autoloaded. You don't need to list your .module and .inc files there.

dependencies[] Since your project is specific to Services 3, you should include the version here.

No licensing or duplication issues.

Waiting time for a full review can be 4 - 6 weeks. You can help reduce your waiting time by contributing to the code review process. Please visit http://groups.drupal.org/code-review for details on how to contribute.

Log in or register to post comments

Comment #2

ccardea CreditAttribution: ccardea commented 12 July 2011 at 17:00

Status:

Needs review

» Needs work

Log in or register to post comments

Comment #3

Robin van Emden CreditAttribution: Robin van Emden commented 13 July 2011 at 09:09

Status:

Needs work

» Needs review

Made the suggested changes. I will contribute to the code review process, as time permits!

Log in or register to post comments

Comment #4

mrfelton CreditAttribution: mrfelton commented 15 July 2011 at 14:40

subs

Log in or register to post comments

Comment #5

mrfelton CreditAttribution: mrfelton commented 16 July 2011 at 16:43

Status:

Needs review

» Reviewed & tested by the community

I've been using the sandbox code, and would like to see this come out of the sandbox on to d.o as a fully fledged project.

Log in or register to post comments

Comment #6

ccardea CreditAttribution: ccardea commented 16 July 2011 at 17:14

Status:

Reviewed & tested by the community

» Needs review

It's counter productive to change the status of the application without doing a real review. It's good to know that the code works and has users, but that's not really a code review

Log in or register to post comments

Comment #7

mrfelton CreditAttribution: mrfelton commented 16 July 2011 at 18:24

Status:

Needs review

» Reviewed & tested by the community

Ok, well I don't see any issues with the code over and above the ones that I reported in the modules issue queue, which have been committed now.

Only one minor code style thing, which is that there is an inconsistancy in the comment style for hook implementations.

e.g.

/**
 * Implements hook_permission().
 */

/**
 * Implementation of hook_services_resources().
 */

They should both be using the Implements hook_hookname(). format. Is that something that would stop this brom breaking out of the sandbox?

Log in or register to post comments

Comment #8

Robin van Emden CreditAttribution: Robin van Emden commented 17 July 2011 at 10:18

Latest commit fixed the inconsistency reported by mrfelton. As can be seen in the previous commits, some small issues were previously reported and fixed in the sandbox issue cue. (As an aside, there is a discussion on this topic here: Code review in application issue or projects sandbox? - so next time around I will "add a comment to the project application issue to let the applicant and other reviewers know that a 'review' issue" )

Log in or register to post comments

Comment #9

ccardea CreditAttribution: ccardea commented 17 July 2011 at 21:09

Status:

Reviewed & tested by the community

» Needs work

line 69: $access = $access; Why is this necessary?

Log in or register to post comments

Comment #10

Robin van Emden CreditAttribution: Robin van Emden commented 18 July 2011 at 07:37

Status:

Needs work

» Needs review

Hi ccardea, now that is a good question. Leftover code, I guess. Line 69 has been deleted.

Log in or register to post comments

Comment #11

g10 CreditAttribution: g10 commented 18 July 2011 at 14:23

Status:

Needs review

» Needs work

additional changes at #1222098: Invitation to merge with branch on GitHub

Log in or register to post comments

Comment #12

Robin van Emden CreditAttribution: Robin van Emden commented 19 July 2011 at 14:14

Status:

Needs work

» Needs review

Merged with g10's GitHub branch (thanks g10!). Cleaned up the code according to Coder module review info. Reverted to the default args implementation in stead of g10's named args, since they caused tests to fail on our test-server.

Also decided to revert to dependencies[] without (3.x), because of #1013302: Versioned dependencies fail with dev versions and git clones . Formally ccardea is right in #1, but adding (3.x) causes lots of headaches for drush and git users. Will add (3.x) to dependencies[] if and when this issue is resolved.

Log in or register to post comments

Comment #13

ccardea CreditAttribution: ccardea commented 20 July 2011 at 16:39

g10 appears to be much more familiar with your code than I am. If he's happy with the changes, he should mark the application RTBC

Log in or register to post comments

Comment #14

g10 CreditAttribution: g10 commented 21 July 2011 at 14:45

Status:

Needs review

» Reviewed & tested by the community

After reviewing the module in depth, I can confirm the current version (merged w/ the changes from the github branch) does what it claims,
I do not see a reason to hold it further in application state

Log in or register to post comments

Comment #15

Robin van Emden CreditAttribution: Robin van Emden commented 21 July 2011 at 17:25

File	Size
amfserver_trace.txt	1.11 KB
main_menu_rest.png	231.02 KB
non_existing_menu_rest.png	221.39 KB

Thanks, g10.

For general information: attached screen-shots of REST responses returned by "Services Menu" when retrieving an existing and a non-existing menu. Also a text file containing the object returned, a trace from a Flash AS3 app calling AMFServer.

Log in or register to post comments

Comment #16

mlncn CreditAttribution: mlncn commented 25 July 2011 at 12:07

Congratulations! You can now promote sandbox projects to full status ones. When you would like further review for any project please do ask at http://groups.drupal.org/peer-review/requests

Further cleanup issues introduced here and new ones can be filed on your project issue queue. Thank you for your contribution and i look forward to seeing its refinement and development and your continued involvement in Drupal!

_{benjamin, agaric}