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

Revisit and finalize the AST implementation #232

Closed
colinodell opened this issue Feb 3, 2016 · 5 comments
Closed

Revisit and finalize the AST implementation #232

colinodell opened this issue Feb 3, 2016 · 5 comments
Assignees
Labels
discussion Open discussion about a particular feature or proposed change do not close Issue which won't close due to inactivity feedback wanted We need your input!
Milestone

Comments

@colinodell
Copy link
Member

colinodell commented Feb 3, 2016

While the AST implementation is working great, there are some areas I'd like to revisit and see if changes could improve the implementation:

Remove parsing-related methods from node types

Node types contain some methods which are only used for parsing Markdown input:

  • matchesNextLine()
  • handleRemainingContents()
  • etc.

Can we somehow move these into their respective parsers? Or alternatively, do we need to separate node types from node instances (similar to how the Symfony Form Component has "form types" and form objects)?

Implement XML serialization

The reference parser supports rendering the AST as XML. I would like to have the same functionality in league/commonmark. This would also be a good test of using the AST for purposes other than rendering it as HTML (converting MD -> XML, HTML -> MD, etc.) However, we may need to implement the other changes in this list including the next one:

Determine whether node types should be referenced by an (arbitrary) name instead of namespaced class

When registering parsers and renderers, or checking the type of the node, we're currently relying on using PHP classes and namespaces. For example, a paragraph is differentiated from a list by it's class of League\CommonMark\Block\Element\Paragraph.

We've also added the ability for renderers to handle subclasses which relies on the class hierarchy in PHP.

But in order to implement the XML serialization, we need to associate a name like paragraph to this element.

I'd therefore like to investigate whether we can use a system like Symfony's Form Component where each type declares its name (i.e. paragraph) and an optional parent type. We'd then register each type in the environment like so:

Update: Let's keep using fully-namespaced classes and use a different approach to determine serialized XML node names

$environment->addType('paragraph`, new League\CommonMark\Block\Type\Paragraph());

This paragraph name would then be used for things like:

  • Registering parsers/renderers for each type
  • Determining which renderer to use
  • Naming XML elements in the AST
  • Node type inheritance

However, Symfony 3.0 removed the ability to use these "alias" names and now require the fully-qualified class names. I'd like to understand why this was done in order to determine whether this idea is good or not.

Change the namespace from Node to AST

I'm not sure if I want to do this, but I do want to consider it.


I may add more to this list later. Any thoughts or feedback are welcome!

@colinodell colinodell added the feedback wanted We need your input! label Feb 3, 2016
@colinodell colinodell self-assigned this Feb 3, 2016
@colinodell colinodell added the discussion Open discussion about a particular feature or proposed change label Feb 3, 2016
@yellow1912
Copy link

Event Symfony is switching to use namespace class now, so I think what you are using now is fine. As long as the user can pass into either a class name or an object i think we are fine. I would like to have better documents however, right now it's really hard to figure out how things work and connect to each other.

@colinodell
Copy link
Member Author

After thinking this over for a while, I agree with @yellow1912 - I think keeping the full class name would be better, and having improved documentation would be best :)

I'm still very much interested in implementing the other 3 ideas sometime in the (hopefully near) future.

@colinodell colinodell mentioned this issue Oct 1, 2018
12 tasks
@colinodell colinodell added this to the v0.19 milestone Mar 15, 2019
@colinodell
Copy link
Member Author

I'd like to look at the feasibility of doing this for v0.19 if possible.

@colinodell colinodell removed this from the v0.19 milestone Apr 9, 2019
@colinodell colinodell mentioned this issue Apr 18, 2019
10 tasks
@colinodell
Copy link
Member Author

Punting this to v2.0. Yes, the current implementation could be slightly better, but it seems to be good enough for everyone's needs right now, so we'll keep it as-is for v1. We can revisit this later.

@colinodell colinodell added this to the v2.0 milestone Apr 19, 2019
@colinodell colinodell added the do not close Issue which won't close due to inactivity label Apr 20, 2019
@colinodell
Copy link
Member Author

This issue has been replaced by #430 and #431.

colinodell added a commit that referenced this issue May 3, 2020
colinodell added a commit that referenced this issue May 3, 2020
colinodell added a commit that referenced this issue May 3, 2020
colinodell added a commit that referenced this issue May 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Open discussion about a particular feature or proposed change do not close Issue which won't close due to inactivity feedback wanted We need your input!
Projects
None yet
Development

No branches or pull requests

2 participants