3.2/feature/4476 allow md extension #45

Merged
merged 1 commit into from Mar 15, 2012

Projects

None yet

3 participants

Contributor

Links to guide pages can now optionally include the .md suffix - eg [My first page](my_first_page.md) will now correctly map to the my_first_page.md file in your module's userguide. This allows guide files to be browsed on github as well as in the userguide.

See ticket: http://dev.kohanaframework.org/issues/4476

Build Scheduled

Contributor

Please change this to use pathinfo($filename, PATHINFO_EXTENSION) and check for "md" or "markdown", as both are valid Markdown extensions.

Contributor

@shadowhand, I can - but currently the Kohana::find_file call at #45 (comment) enforces a .md extension. Do you want me to change that too - so all three input URLs (page, page.md and page.markdown) map to both possible disk files (page.md, page.markdown)? Or it should use supplied extension if present, and map to .md if no extension is specified as currently?

Contributor

To keep this change small, just modify your current code to check for either .markdown or .md in links. If we decide to use both at a later date, it will be easy. Thanks!

@acoulton acoulton Allow documentation links to include .md suffix [fixes #4476]
Links to guide pages can now optionally include the .md suffix - eg
[My first page](my_first_page.md) will now correctly map to the
my_first_page.md file in your module's userguide. This allows
guide files to be browsed on github as well as in the userguide.
c6ebbe0
Contributor

@shadowhand apologies for the delay, I've been away. Have amended the commit as you requested, and added a test for that method.

Build Scheduled

@shadowhand shadowhand commented on the diff Mar 15, 2012
classes/controller/userguide.php
public function file($page)
{
+
+ // Strip optional .md or .markdown suffix from the passed filename
+ $info = pathinfo($page);
shadowhand
shadowhand Mar 15, 2012 Contributor

This is not what I asked for. Please use something like this: $info = pathinfo($page, PATHINFO_EXTENSION).

acoulton
acoulton Mar 15, 2012 Contributor

@shadowhand sorry, I'm not sure I understand you. I am essentially doing that, but once I've checked whether the page has a .md or .markdown extension I still need to strip that extension off the filename before it gets passed to Kohana::find_file (which will add the extension back on again as you can see at #L320).

I figured it was more reliable and quicker to rebuild the filename without extension using the other pathinfo keys. I guess the alternative would be:

$ext = pathinfo($page, PATHINFO_EXTENSION);
if (($ext =='md') OR ($ext == 'markdown'))
{
    $ext ='.'.$ext;
    $len = strlen($ext);
    $page = substr($page, -$len, $len);
}

but that seems much less tidy than just adding the plain filename back to the path?

Or I suppose, closer to what I'm doing:

$info = pathinfo($page, PATHINFO_EXTENSION);
if (($ext =='md') OR ($ext == 'markdown'))
{
    $page = pathinfo($page, PATHINFO_DIRNAME).DIRECTORY_SEPARATOR.pathinfo($page, PATHINFO_FILENAME);
}

but it seemed more efficient to get all of that at once rather than in three separate calls...

shadowhand
shadowhand Mar 15, 2012 Contributor

Oops, I missed that. You could also just use: $page = substr($page, -(strlen($ext))) rather than recompiling the page name. Your choice.

acoulton
acoulton Mar 15, 2012 Contributor

I figured (maybe unnecessarily) that recompiling with the pathinfo values was more reliable in case of any future/alternative platform that used some alternate form of filenaming - on the basis that pathinfo would always be consistent with itself.

Also, your example isn't quite right I think (mine above isn't either) - it would need to be substr($page,0,-3).

I'm happy with it as is, but if you want me to switch it to use substr I'm happy to do so.

@shadowhand shadowhand merged commit 02a3e21 into kohana:3.2/develop Mar 15, 2012
Contributor

Merged up to 3.3/develop. Thanks @acoulton!

Contributor

You're welcome!

On 15 Mar 2012 16:04, "Woody Gilk" <
reply@reply.github.com>
wrote:

Merged up to 3.3/develop. Thanks @acoulton!


Reply to this email directly or view it on GitHub:
#45 (comment)

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