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

RST Reader - Improved Role Support #1805

Merged
merged 7 commits into from Dec 15, 2014

Conversation

Projects
None yet
3 participants
@bergey
Copy link
Contributor

bergey commented Dec 12, 2014

These changes make Roles behave more like the RST definition, and I hope more useful. I've also added warnings when the RST reader ignores syntax.

  • use Code constructor for :literal: role
  • add custom role name to classes (when possible)
  • allow role directive without parent - use Span constructor

Commit e784d4b refactors custom role handling, but shouldn't change behavior (except order of classes). I think it makes the code easier to understand, but feedback is welcome.

@merijn, do you have time to take a look? I think you were the last to work on custom roles. In particular, I've changed the type of stateRstCustomRoles. Before my changes, the base role was computed when defining a new custom role, and ignored when rendering the role. I don't see any reason (other than maybe debugging) why we need to store both of these. I also don't see how the parent role could depend on the Attrs. Does this look OK, or am I breaking something?

This differs from rst2xml.py in (at least) the following ways, which I'm willing to fix if they bother people. I suspect they're not issues in practice.

  • If a custom role inherits from a custom role, both role names are included in the classes
  • The :span: role is recognized as built-in, to implement custom roles with unspecified parent
  • For unknown roles, pandoc outputs the quoted text, but not the role markup; rst2xml outputs both

bergey added some commits Dec 5, 2014

RST Reader: Warn about skipped directives
move `addWarning` to Parsing.hs, so it can be used by Markdown & RST readers.
expose warnings from RST reader; refactor
This commit moves some code which was only used for the Markdown Reader
into a generic form which can be used for any Reader.  Otherwise, it
takes naming and interface cues from the preexisting Markdown code.
RST: literal role should produce Code,
code role should have "code" class.

http://docutils.sourceforge.net/docs/ref/rst/roles.html says that
`text`:literal` is the same as ``text``.  docutils outputs a <literal>
element in both cases, whereas for the code role, it outputs a <literal>
element with the "code" class.
RST reader: improve support for custom roles
- Add "sourceCode" to classes for :code: role, and anything inheriting
  from it.
- Add the name of the custom role to classes if the Inline constructor
  supports Attr.
- If the custom role directive does not specify a parent role, inherit
  from the :span: role.

This differs somewhat from the rst2xml.py behavior.  If a custom role
inherits from another custom role, Pandoc will attach both roles' names
as classes.  rst2xml.py will only use the class of the directly invoked
role (though in the case of inheriting from a :code: role with a
:language: defined, it will also provide the inherited language as a
class).
RST Reader: compute Attrs when role is defined
Move recursive role lookup from renderRole to addNewRole.  The Attr value
will be the same for every occurance of this role, so there's no reason
to compute it every time.  This allows simplifying the
stateRstCustomRoles map considerably.

We could go even further, and remove the fmt and attr arguments to
renderRole, which are null except for custom roles.
@jgm

This comment has been minimized.

Copy link
Owner

jgm commented Dec 13, 2014

This looks promising to me -- let's wait a few days to see if @merijn chimes in.

@merijn

This comment has been minimized.

Copy link
Contributor

merijn commented Dec 15, 2014

At a glance it looks fine to me. To be honest, I'm not really sure why stateRstCustomRoles was implemented like that, it made sense to me at the time, but I'd been messing around for awhile, so I'm pretty sure that's just accidental artefact of previous attempts.

jgm added a commit that referenced this pull request Dec 15, 2014

Merge pull request #1805 from bergey/rst
RST Reader - Improved Role Support

@jgm jgm merged commit a864e9a into jgm:master Dec 15, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@jgm

This comment has been minimized.

Copy link
Owner

jgm commented Dec 15, 2014

Thanks!

@bergey bergey referenced this pull request Dec 20, 2014

Open

New builder #1

12 of 17 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment