Configurable path for asciidoc script

Hey! nice to see another developer on this queue, and thanks for the patch :-)

I tried to cover all the comments I have for the patch, and I know this seems a little long but I hope you can share my vision about this feature.

+++ b/asciidoc.info
@@ -1,6 +1,14 @@
+; $Id: asciidoc.info,v 1.2 2010/12/18 23:03:13 marvil07 Exp $

$Id$ is not anymore needed since git migration ;-)

+++ b/asciidoc.info
@@ -1,6 +1,14 @@
+
+; Information added by drupal.org packaging script on 2010-12-18
+version = "7.x-1.0-rc1"
+core = "7.x"
+project = "asciidoc"
+datestamp = "1292713848"
+

Please make a diff against current git branch, instead of last dev tarball so that information is not in the patch.

+++ b/asciidoc.install
@@ -1,31 +1,9 @@
+// $Id: asciidoc.install,v 1.2 2010/12/18 23:03:13 marvil07 Exp $

Same $Id$ comment.

+++ b/asciidoc.install
@@ -1,31 +1,9 @@
-/**
- * Implements hook_requirements().
- */
-function asciidoc_requirements($phase) {
-  $t = get_t();
-
-  exec('asciidoc --version', $output, $ret);
-  if ($ret == 0) {
-    $severity = REQUIREMENT_OK;
-    $version = substr(array_shift($output), 9);
-  }
-  else {
-    $severity = REQUIREMENT_ERROR;
-    $version = $t('You need to install <a href="@official_site">AsciiDoc</a> in order to use the AsciiDoc filter module.', array('@official_site' => 'http://www.methods.co.nz/asciidoc/index.html'));
-  }
 
-  return array(
-    'asciidoc' => array(
-      'title' => $t('AsciiDoc'),
-      'value' => $version,
-      'severity' => $severity,
-    ),
-  );
-}

Ok, makes sense to remove the hook_requirements if we are going to require manual configuration.

+++ b/asciidoc.module
@@ -15,10 +16,109 @@ function asciidoc_filter_info() {
+  $highlight_options = drupal_map_assoc(
+    array("source-highlight", "Pygments"), ¶
+    'highlight_options'
+  );
+
+  // Select field for the highlight library
+  $form['asciidoc_highlight_library'] = array(
+    '#type' => 'select',
+    '#title' => t('Highlight library'),
+    '#description' => t('Select the highlight library to be used with ¶
+      asciidoc.'),
+    '#default_value' => variable_get('asciidoc_highlight_library',
+      'source-highlight'),
+    '#options' => $highlight_options,
+  );
+
+  // Text field for the Pygments library path.
+  $form['asciidoc_pygments_path'] = array(
+    '#type' => 'textfield',
+    '#title' => t('Pygments library path'),
+    '#description' => t('The path to the Pygments library. Let the field blank ¶
+      if the library is in the system path.'),
+    '#default_value' => variable_get('asciidoc_pygments_path'),
+    '#states' => array(
+      // Only show this field when the 'asciidoc_highlight_library' select has
+      // the value Pygments.
+      'visible' => array(
+        ':input[name="asciidoc_highlight_library"]' => ¶
+          array('value' => 'Pygments'),
+      ),
+    ),
+    '#size' => 60,
+    '#maxlength' => 200,
+  );

I would like to separate the asciidoc path configuration from otheradditions, so it would be great to have another issue for that explaining why it would be important to have highligh library(a link to somewhere exlaining highligh libraries would be just nice).

+++ b/asciidoc.module
@@ -27,7 +127,12 @@ function _asciidoc_asciidoc_simple_tips($filter, $format, $long) {
+function _asciidoc_filter_asciidoc_simple_process($text, ¶
+    $filter, ¶
+    $format, ¶
+    $langcode, ¶
+    $cache, ¶
+    $cache_id) {

code style chnage is not really needed there.

+++ b/asciidoc.module
@@ -38,11 +143,32 @@ function _asciidoc_process($text) {
+  $asciidoc_highlight_library = variable_get('asciidoc_highlight_library');
+  $asciidoc_pygments_path = variable_get('asciidoc_pygments_path');
+  $asciidoc_asciidoc_path = variable_get('asciidoc_asciidoc_path');
+  ¶
+  $command_str = 'echo %s | ';
+  if ($asciidoc_highlight_library === 'Pygments') {
+    // include the Pygments library in the PATH of the command
+    if ($asciidoc_pygments_path !== '') {
+      $command_str .= 'PATH=$PATH:' . $asciidoc_pygments_path;
+    }
+    ¶
+    // indicate to the Asciidoc that we are using the Pygments filter
+    $command_str .= ' ' . $asciidoc_asciidoc_path . ' -a pygments';
+  } else {
+    $command_str .= $asciidoc_asciidoc_path; ¶
+  }
+  ¶
+  $command_str .= ' --no-header-footer -o - -';

I guess you are in good path to solve it(I did not really tried the patch yet). But I would really recommend to take a look to other related patch I have done some time ago for versioncontrol_git project #1001668-6: Configurable path for the git binary.

Hey, sorry for the delay here.

I would say we are in the right direction, but there are some more comments I would like to address before committing it.

General comments

• Please remove code changes about highlighting.
• Please take a look to spacing errors(i.e. spaces at the end of a line or spaces on an empty line)

Specific comments

+++ b/README.txt
@@ -1,8 +1,6 @@
-
 = AsciiDoc filter =
 
-This module provides AsciiDoc filter integration for Drupal input
-formats.
+This module provides AsciiDoc filter integration for Drupal input formats.
 
 == Features ==
 
@@ -18,8 +16,7 @@ To submit bug reports and feature suggestions, or to track changes:

@@ -18,8 +16,7 @@ To submit bug reports and feature suggestions, or to track changes:
 
 == Instalation ==
 
-* Install as usual, see http://drupal.org/node/895232 for further
-information.
+* Install as usual, see http://drupal.org/node/895232 for further information.
 
 == Configuration ==
 

I do not really understand the formatting changes. IMHO they are not necessary.

+++ b/README.txt
@@ -18,8 +16,7 @@ To submit bug reports and feature suggestions, or to track changes:
@@ -36,4 +33,6 @@ information.

@@ -36,4 +33,6 @@ information.
 
 The authors of AsciiDoc.
 
-Current Maintainer: Marco Villegas (marvil07)
+.Current Maintainers: ¶
+- Marco Villegas (marvil07)
+- kenjiru <kenjiru.ro@gmail.com>

Author != co-maintainer: Don't get me wrong, I'll be glad to make you co-maintainer, but not yet. On why, please take a look to http://drupal.org/node/363367

+++ b/asciidoc.admin.inc
@@ -0,0 +1,93 @@
+  $highlight_options = drupal_map_assoc(
+    array("source-highlight", "Pygments"), ¶
+    'highlight_options'
+  );
+
+  // Select field for the highlight library
+  $form['asciidoc_highlight_library'] = array(
+    '#type' => 'select',
+    '#title' => t('Highlight library'),
+    '#description' => t('Select the highlight library to be used with ¶
+      asciidoc.'),
+    '#default_value' => variable_get('asciidoc_highlight_library',
+      'source-highlight'),
+    '#options' => $highlight_options,
+  );
+
+  // Text field for the Pygments library path.
+  $form['asciidoc_pygments_path'] = array(
+    '#type' => 'textfield',
+    '#title' => t('Pygments library path'),
+    '#description' => t('The path to the Pygments library. Let the field blank ¶
+      if the library is in the system path.'),
+    '#default_value' => variable_get('asciidoc_pygments_path'),
+    '#states' => array(
+      // Only show this field when the 'asciidoc_highlight_library' select has
+      // the value Pygments.
+      'visible' => array(
+        ':input[name="asciidoc_highlight_library"]' => ¶
+          array('value' => 'Pygments'),
+      ),
+    ),
+    '#size' => 60,
+    '#maxlength' => 200,
+  );

Again, highlight should be other issue, so not in this patch.

+++ b/asciidoc.admin.inc
@@ -0,0 +1,93 @@
+function asciidoc_config_form_validate($form, &$form_state) {

Please take a look to the _versioncontrol_git_binary_check_path() function on the patch I mentioned earlier. I would like to have the same logic here, so we can report to the user what's the exact problem.

+++ b/asciidoc.info
+++ b/asciidoc.info
@@ -4,3 +4,5 @@ core = 7.x

@@ -4,3 +4,5 @@ core = 7.x
 
 files[] = asciidoc.install
 files[] = asciidoc.module
+
+

unnecessary hunk

+++ b/asciidoc.module
@@ -1,48 +1,94 @@
-      'title'            => t('Simple AsciiDoc filter'),
+      'title'            => t('AsciiDoc filter'),
       'description'      => t('Allows users to use AsciiDoc syntax.'),
-      'process callback' => '_asciidoc_filter_asciidoc_simple_process',
-      'tips callback'    => '_asciidoc_asciidoc_simple_tips',
+      'process callback' => 'asciidoc_process_callback',
+      'tips callback'    => 'asciidoc_filter_tips',
+      'settings callback' => 'asciidoc_filter_settings_callback',

The reasoning behind the having a "Simple AsciiDoc filter" fileter instead of just an "AsciiDoc filter" filter, is to be explicit about the target of the filter, so, maybe we can implement other filters inside this module instead of having one big super-configurable filter.

So, I would say to stay with the explicit "simple" on the name and the callbacks.

BTW I'm OK with changing callback names to get rid of "_" prefix(IIRC it started because of the the existence of D7 hook_process).

+++ b/asciidoc.module
@@ -1,48 +1,94 @@
+  $asciidoc_highlight_library = variable_get('asciidoc_highlight_library');
+  $asciidoc_pygments_path = variable_get('asciidoc_pygments_path');
+  $asciidoc_asciidoc_path = variable_get('asciidoc_asciidoc_path');

New variables implies two things:
- Give a default on every variable_get.
- Remove those variables on uninstall.

+++ b/asciidoc.module
@@ -1,48 +1,94 @@
+  $output = "<div class='asciidoc'>\n" . $output . "\n</div>\n";
 

Why adding a div?

IMHO that would not be clear for any user where that div come from.

BTW, when you attach a new patch you usually mark it as "need review" ;-)