Better error handling

Implement validate hook to check that both arguments are not-null.

+++ b/grn.drush.inc
@@ -62,6 +62,15 @@ function grn_drush_command() {
+    drush_set_error('DRUSH_GRN_MISSING_ARG', dt('Two valid Git tags are required as arguments.'));

It should return this. drush_set_error always returns false, so you could just prefix the line with return. That way the command will be canceled.

Powered by Dreditor.

Actually if a command validation sets an error it will also halt the command. See drush_invoke_args().

As a coding standard issue, I'm not actually clear on whether it is preferable to return, so I will leave the status alone.

Oh. That makes sense. I'd say it's fine then, so back to needs review. I'll be reviewing patches for grn this weekend.

+++ b/grn.drush.inc
@@ -62,6 +62,15 @@ function grn_drush_command() {
+ * Implementation of drush_hook_COMMAND_validate().

"Implements".

+++ b/grn.drush.inc
@@ -62,6 +62,15 @@ function grn_drush_command() {
+  if (is_null($tag1n) || is_null($tag2n)) {

1) Always use !isset() instead of is_null().

2) empty() would be much more appropriate here.

Powered by Dreditor.

Out of curiosity, are we backporting D7 coding standards to D6 projects, or are we just treating this as a D7 project because it should be?

Drupal coding standards are version-independent and "always-current". All new code should follow the current standards, regardless of (core) version. Existing code in older versions may be updated, but doesn't necessarily have to be. Especially for larger code-bases (like Drupal core), updating the code of a previous version for current standards may too huge of a task. However, code in current versions should follow the current standards.

Lastly, when following this practice, make sure to not squeeze coding standards updates/clean-ups into otherwise unrelated patches. E.g., as in this case, the diff context in the patch shows another unrelated, old phpDoc summary. To update existing code, always create a separate and dedicated issue + patch.

sun: Just an fyi, I reposted your comment about coding standards above into #983268-59: Use @implements Doxygen directive for hook implementations, because of the discussion about backporting coding standards.

sun asked me on IRC if #9 was correct. I believe it is correct, but I don't know that it's been formalized. We update coding standards regularly but don't usually go back and update past versions of Drupal to follow them (and Coder module generally follows the current standards only in the latest version, which can cause trouble when it's running its checks).

This is really good information to make clear. Thank you.

I always thought coding standards were a little bit hazy about this issue, but that ultimately binding a version of the standards to a Drupal core version dependency was a good idea for consistency.

Any case, I'm not specifically claiming ownership of this issue, I will try to cycle back around in the next week or two and push any issue I've touched in grn that needs it.

I believe that this actually sums all the patch recommendations nicely, tested on local setup and it works as expected.

+++ b/grn.drush.inc
@@ -99,6 +99,15 @@ function drush_grn_release_notes($tag1n, $tag2n) {
+    drush_set_error('DRUSH_GRN_MISSING_ARG', dt("  Usage: drush release-notes START_POINT END_POINT\n  Try 'drush release-notes --help' for more information."));

I prefer <start> <end>, though this is likely to change to <end> (<start>)

@ #15: Can you please give me more details on your patch review so that I can fix my patch ?

Sorry, darn corner braces :)

I suppose this command usage format is the valid one until #1253356: use two most recent tags as defaults is completely resolved and in the code.

I do realize that we need to use one single reference style for the arguments of the function so that everything is coherent. If it is <start> <end> then I'd be happy to go ahead and fix the patch ;)

I think those are the most comprehensible to less git-savvy folks at a glance. We can go on to document that start and end refer to git treeishes etc etc in the README and comments.

Ok, the patch is now using <start> and <end>.

Hehe, never new --help existed! Looks good, will test just to make sure everything is copacetic.

http://drupalcode.org/project/grn.git/commit/d29f595