Fix (and improve) hook_reviews

douggreen - September 17, 2008 - 19:34
Project:Translation template extractor
Version:6.x-2.x-dev
Component:Code
Category:bug report
Priority:normal
Assigned:Unassigned
Status:closed
Description

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.

AttachmentSize
potx-coder.patch1.48 KB

#1

douggreen - September 18, 2008 - 12:54

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

AttachmentSize
309875.patch 2.5 KB

#2

Gábor Hojtsy - September 19, 2008 - 08:59
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 :)

#3

Gábor Hojtsy - September 24, 2008 - 10:09

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

#4

stella - September 24, 2008 - 15:28

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

#5

Gábor Hojtsy - September 24, 2008 - 16:07

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.

AttachmentSize
potx_status_refactoring_for_coder_integration.patch 7.51 KB

#6

stella - September 24, 2008 - 18:30
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.

#7

Gábor Hojtsy - September 24, 2008 - 19:15
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.

#8

Gábor Hojtsy - September 26, 2008 - 10:53

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.

AttachmentSize
potx_numering.patch 698 bytes

#9

Gábor Hojtsy - October 13, 2008 - 21:47

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

#10

Gábor Hojtsy - August 24, 2009 - 13:06
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.

#11

System Message - September 7, 2009 - 13:10
Status:fixed» closed

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

 
 

Drupal is a registered trademark of Dries Buytaert.