I slightly changed the coder api just before DrupalCon Szeged, in order to better support error message from the drush command line. Sorry, I should of publicized this but didn't. But these changes broke the potx_reviews handling of errors. The attached patch not only fixes this problem, but improves the error messages generated by the potx review.

Comments

douggreen’s picture

StatusFileSize
new2.5 KB

This patch does an even better job at converting the potx warnings into coder messages.

gábor hojtsy’s picture

Status: Needs review » Needs work

Great, thanks for putting in effort to propagating the changes :) I am glad to implement the coder review hook and help people fix translatability issues. I have a few comments on the patch:

- There is no guarantee that the translation of the error message will keep the $file:$line notation, so we need to move to %location and then replace %location with $file:$line to ensure this format is kept.
- Same applies for your matching of ' in ...' which is based on the English original string. I'd suggest you match for : and whitespace followed by non-whitespace stuff before it and numbers after it. That should identify the file name/line properly regardless of translation.
- Although possibly rare, the theming of %filename could also use something else but <em>, so it might make sense to look at the theme('placeholder') processed version.
- There are other instances of potx_error() which would need fixing with this in mind.

Thanks again for helping fix the code review code! I hope my suggested improvements make this change more translation and theme agnostic, and that you can keep up the work to improve the patch :)

gábor hojtsy’s picture

Also on better coder integration via providing better error messages to aid developers here: http://drupal.org/node/312523 (related, not duplicate)

stella’s picture

I don't have time to help improve on the patch, but I can certainly review / test future instalments.

gábor hojtsy’s picture

Ok, let's check this one out. It introduces a new POTX_STATUS_STRUCTURED status handling mode, in which case, it collects the error messages, line numbers and file names in a structured array, not in blobs of text. This also led to generalization in the error messages. Now all of them have the file name and line at the end in non-structured reporting modes, appended automatically.

I've also fixed phpdoc on potx_status() and the status constants, and used the wording "Invalid localization code" instead of "Invalid marker content". We have http://drupal.org/node/312523 for better error messages, so I am not going to go into that right now, this was a logical and slight improvement.

The coder integration gets much simpler due to these changes, so it does not need to reverse engineer the file and line number from the text, it does not need to remove these, and it does not need to replace stuff like newlines and asterisk marks, which were only there for the command line reporter originally and kept being there ever since. Way better IMHO. This clears out all my concerns from #2, so as long as coder folks can verify this works fine, I am happy to commit this.

stella’s picture

Status: Needs work » Reviewed & tested by the community

Excellent! That works well. :)

However, the patch from #312523: Better error messages for potx errors / code reviews. now no longer applies. So will re-roll that one.

gábor hojtsy’s picture

Version: 6.x-2.x-dev » 5.x-2.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Great, committed to the 6.x-2.x branch, so we can keep going with the better messages there. Moving on to http://drupal.org/node/312523

Since the new potx_status() API was introduced in 6.x-2.x, it might be pointless to backport these changes to 5.x, since it would result in highly separate code paths. Marking to be ported to 5.x-2.x for now, and we will see later.

gábor hojtsy’s picture

StatusFileSize
new698 bytes

Doh. A considerable glitch in this patch was that coder numbers lines from 0 and potx does from 1 (as in most editors translators would use), so we need to decrement the line number we provide coder with or we get the next line as part of the error message (as my test showed). Applied this follow up patch and it runs much better now.

gábor hojtsy’s picture

Also related comments and screenshots on http://drupal.org/node/312523#comment-1057817

gábor hojtsy’s picture

Version: 5.x-2.x-dev » 6.x-2.x-dev
Status: Patch (to be ported) » Fixed

Drupal 6 already has the above patch committed, and Drupal 7 has different code structure for finding out the line number. Also, the Drupal 5 version is not maintained anymore, so marking back to fixed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.