Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow parsedown to specify list start attribute #431

Merged
merged 17 commits into from
Oct 5, 2016

Conversation

aidantwoods
Copy link
Collaborator

(I think this should work)
Allow parsedown to specify list start attribute (see: #100 (comment))

(I think this should work)
Allow parsedown to specify list start attribute (see: erusev#100 (comment))
@aidantwoods aidantwoods changed the title Update Parsedown.php Allow parsedown to specify list start attribute Sep 26, 2016
oops
Attempting to determine which function change is causing test jobs to fail (in unexpected ways)
Looks like I might need to return the pattern which was used previously
Reverting last change as build still failed

This build will still fail, but I'm hoping it will only fair where the list start value has been inserted
@aidantwoods
Copy link
Collaborator Author

Okay this looks good.

There is 1 failure (which is entirely by design – to add the new feature)

screen shot 2016-09-27 at 01 25 15

@PhrozenByte
Copy link
Contributor

@erusev: Increases CommonMark compliance, see http://spec.commonmark.org/0.26/#start-number (however, I didn't check the suggested code changes)

Okay, so maybe I should have looked 20 lines or so above where I made the edit in the element function – looks like it already supports adding attributes ;p
Have amended the change to blocklist to use the already existing functionality, and have reverted the change that I made to the element function.
@aidantwoods
Copy link
Collaborator Author

I've removed a change I made that added a feature that already exists ;p
Have rewritten to use the existing functionality – guess that's what I get for skimming the code ;p

Checks still look good – still receiving (the expected) single failure

@PhrozenByte
Copy link
Contributor

PhrozenByte commented Oct 4, 2016

Can you please update the test? You just have to edit test/data/ordered_list.html 😃

Looks great to me! 👍

@aidantwoods
Copy link
Collaborator Author

Okay, this should be good (including tests).
Had to modify the test via file upload, as GitHub auto-appends a new line when saving o.0

@erusev
Copy link
Owner

erusev commented Oct 4, 2016

Wouldn't it work if instead of line 505, you add the brackets on 506 and keep 510 unchanged?

image

@aidantwoods
Copy link
Collaborator Author

@erusev Sort of – the dot still needs to be excluded somehow, whether that's by grabbing all the numbers in the capture group, or by chopping off the last character (the dot).

These would be the changes for the former method
screen shot 2016-10-04 at 21 20 35

@erusev
Copy link
Owner

erusev commented Oct 4, 2016

You are right.

How about this - 505 and 510 stay the same, but in the if at 516, work with the existing match and split it by "." to get the number.

image

@aidantwoods
Copy link
Collaborator Author

@erusev in-fact ignore my previous comment – that wouldn't work, since the number needs to be known before inserting it (to check if it is 1)

It would actually need to be more like this (with the string splitting to get the dot)

screen shot 2016-10-04 at 22 00 49

@erusev
Copy link
Owner

erusev commented Oct 5, 2016

This looks much better.

Could you update the pull request?

Thanks.

@aidantwoods
Copy link
Collaborator Author

@erusev Done – as written in the screenshot, bar the explode function – build tests indicate PHP 5.3 had trouble navigating the array as it was returned by explode (guessing I'd have to use an intermediary variable – which impacts readability slighly IMO). So I've just used a preg_replace in its place to grab all the digits before the first ..

@erusev
Copy link
Owner

erusev commented Oct 5, 2016

preg_replace is very slow, though - check this out: http://stackoverflow.com/questions/1744956/explode-and-get-a-value-in-one-line-of-code

Performance: Swap preg_replace for stristr to obtain list start
@aidantwoods
Copy link
Collaborator Author

@erusev looks like stristr is working as expected 😃 any other changes?

@erusev
Copy link
Owner

erusev commented Oct 5, 2016

Great, a few more :)

  1. We don't use _ as a word separator
  2. No need to remove empty lines
  3. It'd be more consistent if the if block is surrounded by empty lines

Syntax preferences to match surrounding code
Remove github added tabs on blank lines
@aidantwoods
Copy link
Collaborator Author

@erusev Syntax has been adjusted ;-)

@erusev
Copy link
Owner

erusev commented Oct 5, 2016

Could you handle 2 more :)

  1. Single quotes on "."
  2. Empty line before the nested if

@aidantwoods
Copy link
Collaborator Author

@erusev Okay, that should be done now? 😜

@erusev
Copy link
Owner

erusev commented Oct 5, 2016

Nice, that's perfect 🎉

@erusev erusev merged commit f4e0234 into erusev:master Oct 5, 2016
@aidantwoods
Copy link
Collaborator Author

@erusev Nice! 😃 Guess it's probably safe to close #100 and its duplicates now

@erusev
Copy link
Owner

erusev commented Oct 6, 2016

Yep :) Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants