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

Add #name to renderers #377

Merged
merged 3 commits into from
Mar 8, 2015
Merged

Add #name to renderers #377

merged 3 commits into from
Mar 8, 2015

Conversation

bkeepers
Copy link
Contributor

Closes #374

From the tests:

    assert_equal "markdown", GitHub::Markup.renderer('README.md').name
    assert_equal "redcloth", GitHub::Markup.renderer('README.textile').name
    assert_equal "rdoc", GitHub::Markup.renderer('README.rdoc').name
    assert_equal "org-ruby", GitHub::Markup.renderer('README.org').name
    assert_equal "creole", GitHub::Markup.renderer('README.creole').name
    assert_equal "wikicloth", GitHub::Markup.renderer('README.wiki').name
    assert_equal "asciidoctor", GitHub::Markup.renderer('README.adoc').name
    assert_equal "restructuredtext", GitHub::Markup.renderer('README.rst').name
    assert_equal "pod", GitHub::Markup.renderer('README.pod').name

/cc @aroben @github/user-content

@mojavelinux
Copy link
Contributor

Shouldn't the name for AsciiDoc files be "asciidoc" instead of "asciidoctor" to follow the pattern?

@ymendel
Copy link
Contributor

ymendel commented Sep 30, 2014

Shouldn't the name for AsciiDoc files be "asciidoc" instead of
"asciidoctor" to follow the pattern?

I think asciidoctor makes sense, with analogy to textile/redcloth.

@bkeepers
Copy link
Contributor Author

Shouldn't the name for AsciiDoc files be "asciidoc" instead of "asciidoctor" to follow the pattern?

I had the same question myself. Initially I wrote it to be the name of the format. The original motivation in #374 is for a name to use in cache keys. In that case, I think it makes sense to use the name of the library instead of the format.

@aroben
Copy link

aroben commented Sep 30, 2014

I can see arguments in either direction. Given the currently proposed API (Renderer#name), I think we should be returning the names of the renderers themselves, not the language they render.

@bkeepers Are there any renderers that handle multiple languages currently? If so then returning language names would probably be better. This is all a bit hypothetical, but say you had a renderer that handles both Markdown and Textile documents. If we put the renderer's name (not the document's language) in the cache key, we wouldn't re-render when you renamed a file from README.md to README.textile.

@bkeepers
Copy link
Contributor Author

Are there any renderers that handle multiple languages currently?

Nope. They might handle multiple file extensions, but their behavior doesn't change if the file extension changes (e.g. .markdown to .md).

@@ -44,6 +44,10 @@ def render(content)
@renderer.call(content)
end

def name
"markdown"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should return the name of the gem that will be used for rendering? Presumably the different gems produce different rendering for at least some documents.

@bkeepers
Copy link
Contributor Author

bkeepers commented Jan 2, 2015

@aroben is this still needed?

@aroben
Copy link

aroben commented Jan 2, 2015

It would still be useful to have. We have a workaround for it currently as described in #374. The situation hasn't changed since I filed that issue.

* origin/master: (57 commits)
  tweaks
  Call out CSS issues
  Fix link to linguist
  Release 1.3.3
  The command is POSIX, not Posix
  Release 1.3.2
  Spruce up old tests
  Add empty `a` to support contents with sectnums
  Add comments to describe these calls
  No longer true
  Restore "test" as the default rake task
  point to other places before creating issues
  Extconf hack no longer necessary
  Don't test jdk7
  Fallback on open3 if posix-spawn is unavailable.
  Define CommandImplementation#execute conditional on presence of Posix::Spawn
  Get html_equal tests running with minitest
  Updated deprecated .exists? in markup.rb
  Added the link to Pod::Simple on CPAN.
  Fix pod output
  ...

Conflicts:
	lib/github/markup.rb
	test/markup_test.rb
bkeepers added a commit that referenced this pull request Mar 8, 2015
@bkeepers bkeepers merged commit 46010af into master Mar 8, 2015
@bkeepers bkeepers deleted the implementation-name branch March 8, 2015 03:57
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.

Would like API to determine the language a file will be rendered as
4 participants