Coding standards cleanup [#916560]

Comment	File	Size	Author
	mail_edit.coder_.patch	17.84 KB	jaydub

Comment #1

salvis

he/his

commented 20 September 2010 at 02:19

Thanks!

In what order should your patches be applied? This one first, then re-roll the others?

Log in or register to post comments

Comment #2

jaydub commented 20 September 2010 at 04:51

Good question. Normally I post patches against straight from CVS code for each one so that they can be considered independently. I think this one would be good then I can rerool the others off of updated CVS.

Log in or register to post comments

Comment #3

salvis

he/his

commented 20 September 2010 at 17:23

Status:

Needs review

» Needs work

I agree with most of your changes except for three kinds:

1. Comments: if we touch the comments, then let's do it right, meaning start with a capital letter and end with a dot.

2. Removal of commented trace statements. I know that the guidelines say they should be removed, but I disagree. There's a certain investment in figuring out the appropriate information that is relevant at any given point and how to present it. Having the trace statements ready for uncommenting can be a big help for someone who doesn't know the code and is trying to do something with it. Their formatting should be cleaned up, but if you insist on removing them, then litwol will have to decide...

3. The strings in user_mailkeys(): I suspect that the leading spaces are intentional to distinguish them from user.module's strings and allow separate translation. Maybe it's just a debugging aid, then it should be fixed, or there's a reason, then that should be documented through a comment.

^ litwol?

Log in or register to post comments

Comment #4

jaydub commented 20 September 2010 at 18:52

Regarding #3, the output of the Coder module when selecting the translations checkbox in the coder options resulted in the message about t() and leading/trailing spaces. It's possible that the leading space is for the reason you mention but that seems to be a rather confusing way to do it compared to just adding a prefix/suffix that indicates that Mail Edit owns that string. There is another issue in the queue about some strings not showing up in their translated state as well which may have had something to do with the leading space.

As for debugging well personally I prefer to have release ready module code to be free of debugging code. I think a better alternative would be an internal module debugging flag and a debug function to make it more of a useful debug tool rather than requiring someone to go and uncomment code. As for inline comments, I'm good with whatever is decided.

I'm the owner of the Email Confirm module and there have been a number of users asking for integeration with Mail Edit so that's how I came to the queue to kick the tires. I'd say that anything that leads to a 6.x release of Mail Edit would be good :)