bradchoate / text-textile

Text::Textile -- Perl module for handling Textile format
20 stars 11 forks source link

Support html entities containing capitals or numbers #7

Closed yacomink closed 10 years ago

yacomink commented 10 years ago

Support mixed-case html entites, such as the capital letter Ø, as well as named entities with numbers like ¼

bradchoate commented 10 years ago

First, thank you for the pull request. And I apologize for ignoring it for so long. I'm reviewing it now, and there are two things.

First, you're issuing a single pull request that implements two different things. The change to disable inline formatting rules and changes to entity parsing should be separate pull requests.

Second, the changes to entity parsing look OK, but I'm not inclined to merge the option to disable inline formatting as it really changes what "Textile" is when enabled. If you want to write markup without it being processed, you can escape the formatting characters like this: ==*sigh*==. Another alternative would be to subclass Text::Textile and add such an option that way.

package MyTextile;

use strict;
use warnings;

use Text::Textile;
use base 'Text::Textile';

sub new {
    my $class = shift;
    my %options = @_;
    my $self = $class->SUPER::new(@_);
    $self->{disable_inline_formatting} = 1 if $options{disable_inline_formatting};
    return $self;
}

sub disable_inline_formatting {
    my $self = shift;
    $self->{disable_inline_formatting} = shift if @_;
    return $self->{disable_inline_formatting};
}

sub format_inline {
    my $self = shift;
    my %args = @_;
    if ($self->{disable_inline_formatting}) {
        return $args{text};
    }
    return $self->SUPER::format_inline(@_);
}

This is over-simplified, but I'd rather make changes to Textile's format_inline routine that would make it more suitable for subclassing than to alter the core to make it handle variations of the core syntax differently.

yacomink commented 10 years ago

Hey Brad,

Apologies for combining the two pulls there. I pushed the changes for the inline formatting to my master having forgotten that I had a pull request from my fork already pending for the entity parsing from a few months prior.

You make a good point about sub-classing. I've pitched using ==sigh== to our editors, but oddly they seem to want to use the raw asterisk form far more often than actual bold, and find that syntax unwieldy. They are probably in the minority there though.

Would you be ok with me pulling out the part I've conditionalized in format_inline into a separate sub in Textile.pm so that I can override in my subclass? These features are core to textile, but they are also separated as 'inline formatting' in the perl module docs and 'phrase modifiers' elsewhere, so I don't think it's unreasonable to separate them in code.

Thanks, Andy

On Mon, Jul 7, 2014 at 11:31 AM, Brad Choate notifications@github.com wrote:

First, thank you for the pull request. And I apologize for ignoring it for so long. I'm reviewing it now, and there are two things.

First, you're issuing a single pull request that implements two different things. The change to disable inline formatting rules and changes to entity parsing should be separate pull requests.

Second, the changes to entity parsing look OK, but I'm not inclined to merge the option to disable inline formatting as it really changes what "Textile" is when enabled. If you want to write markup without it being processed, you can escape the formatting characters like this: ==sigh==. Another alternative would be to subclass Text::Textile and add such an option that way.

package MyTextile;

use strict; use warnings;

use Text::Textile; use base 'Text::Textile';

sub new { my $class = shift; my %options = @; my $self = $class->SUPER::new(@); $self->{disable_inline_formatting} = 1 if $options{disable_inline_formatting}; return $self; }

sub disable_inline_formatting { my $self = shift; $self->{disable_inlineformatting} = shift if @; return $self->{disable_inline_formatting}; }

sub formatinline { my $self = shift; my %args = @; if ($self->{disable_inline_formatting}) { return $args{text}; } return $self->SUPER::formatinline(@); }

This is over-simplified, but I'd rather make changes to Textile's format_inline routine that would make it more suitable for subclassing than to alter the core to make it handle variations of the core syntax differently.

— Reply to this email directly or view it on GitHub https://github.com/bradchoate/text-textile/pull/7#issuecomment-48194267.

bradchoate commented 10 years ago
Would you be ok with me pulling out the part I've conditionalized in

format_inline into a separate sub in Textile.pm so that I can override in my subclass?

Yes, let's try that route and see what it looks like.