Support for 1.1 as ordered list, rid off PHP warnings #91

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
2 participants

Support for 1.1 as ordered list:

image

and some PHP warnings

Michelf/Markdown.php
- foreach ($headers as $n => $header)
- $text .= " <th$attr[$n]>".$this->runSpanGamut(trim($header))."</th>\n";
+ foreach ($headers as $n => $header) {
+ $tmp = @$attr[$n];
@michelf

michelf Apr 25, 2013

Owner

Last time I checked, the error suppression operator (@) is very slow. Please don't use it, especially in a loop.

@michelf

michelf Apr 25, 2013

Owner

I often use a reference assignment in those cases were reference semantics don't get in the way. There are plenty of those in PHP Markdown. Might be a better solution.

@jagermesh

jagermesh Apr 25, 2013

Ok, you can do this way instead
$tmp = isset($attr[$n])?$attr[$n]:'';

Owner

michelf commented Apr 25, 2013

I guess I'm missing the point. This doesn't affect the output in anyway, so why would you want to write that instead of the current nested list syntax? Looks rather pointless to me.

In current version such list renders like on screenshot:

image

with my change it renders like this:

image

Owner

michelf commented Apr 25, 2013

Doesn't answer the question. Why write 1.1.1 in the first place if you can write just 1? Where do such list come from?

In my particular case we are:

  • converting project documentation from different DOC/PDF files to Makrdown and some lists are looks wrong without a lof of manual changes
  • we are importing some RSS feeds for our internal Knowledge Base and convert them to Markdown automatically - it also has issues with such numbered lists

And finally - isn't this a regular numbered list? Why it can't looks good once it rendered into Markdown?

Again 1.1.1
I think no need to consider chars here
Owner

michelf commented Apr 25, 2013

I'm still not sure I understand, if you're converting some documents from a given format to Markdown, why can't you just emit regular Markdown syntax instead? Why do your emitted Markdown version need to be in the 1.1 style? Or are you making all the changes manually and are finding it tiresome to convert instances of 1.1 to 1. ?

Also, it seems like there's only one Markdown implementation supporting this 1.1 syntax for lists currently (Pandoc). If I add it to PHP Markdown it'll add another feature that works differently across implementations and increase the pressure for others to do so as people who use the feature will have the expectation it works elsewhere too. I'm really not sure it's worth the trouble.
http://johnmacfarlane.net/babelmark2/?normalize=1&text=1.+test%0A+++1.1.+test

Yes, people doing changes manually. It's a bunch of docs, they are moving all their product user guides to my service at http://docscamp.com and reporting issues with these annoying numbered lists :)

It's up to you of course, but I really think example below must be rendered as numbered list.

  1. test

    1.1. test

and I don't think there are any circumstances when this logic may fail or be in conflict with something else.

Owner

michelf commented Apr 25, 2013

Of course there's situations where this can cause surprises:

Steps to do reproduce some bug:

1.  Update your application to version
    3.2.1. That's the latest version.
2.  Refresh.

Granted, if you replace 3.2.1 by a single number the problem already exists. But that's well known and documented and works the same for all implementations.

Looks fine for me. Not perfect. But it's numbered list at least...

image

Owner

michelf commented Apr 25, 2013

Not fine. What you're missing is that the 3.2.1 in the example above is the continuation of the previous line of text, not intended as a new list item (you need to actually read the text to catch that).

I don't think that's a fatal flaw in itself, but the general rule in Markdown is to keep things simple. The more clever we try to be, the more surprises will be "found" accidentally by people not intending to use one or another alternative syntax. It's better to limit the alternative syntaxes to a minimum, and to case where there's a significant benefit.

In this case I see no benefit in using a 1.1.1 syntax. Not only no benefit, but there's also the downside that for most people the expectation will be a web page showing "1.1" as the list item number, but that won't be the case unless they're using a clever style sheet using CSS counters. So I believe this should not be supported.

Well it's good when written properly.

image

And of course you can do crazy things like below to broke standard behaviour :)

image

In 99% cases it renders good and as expected.

Owner

michelf commented May 2, 2013

The major point is that it will never render as 1.1 in HTML (except for those doing complicated CSS tricks). I think that alone makes it worth rejecting.

The second point is that it'll make it behave differently from most other implementations in the edge case I mentioned. I know this edge case already applies in some cases (as you just demonstrated) but those work the same in other Markdown implementations. I'd rather not add differences unless the value added is great.

If you want to use your custom parser, that's perfectly fine with me. I just think it'd be more trouble to give that to everyone than it is worth.

@michelf michelf closed this May 2, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment