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

Enhancement/types refactor for 2.0 #685

Merged
merged 223 commits into from
Apr 2, 2019

Conversation

aidantwoods
Copy link
Collaborator

@aidantwoods aidantwoods commented Dec 5, 2018

Split Parsedown's Blocks and Inlines into separable components backed by an interface—this should for a good basis for the new "extension" model for adding custom behaviours.

The general idea will be that "extensions" can implement their own Blocks and Inlines, as well as their own Configurables to allow the setter-like behaviour we have currently.
The core Parsedown class now really just strings everything together, only really depending on the basic Paragraph and Plaintext components directly (we could remove the direct dependence if there is a good case for it though)—everything else can be swapped out by the user, so they could go so far as to pick and choose different Blocks and Inlines from various different sources if they so wish.

Perhaps another change worth mentioning is that parsing of particular components is now completely decoupled from the HTML generation. This means that there is no need (and you should not try!) to traverse the HTML structure produced by a particular component if you want to defer to it. Instead each component can present it's own API which makes sense uniquely to that component, and other code that wants to defer parsing can use that API—but not need to depend on the exact structure HTML that will be produced (which is really very subject to change!).
For an example of this, inline Images defer some of their parsing to the Link component
https://github.com/aidantwoods/parsedown/blob/8a48dcfe315bc7e37028626646291538dce24d10/src/Components/Inlines/Image.php#L45-L51
and then store that produced Link for later use, where the basic attributes are retrieved https://github.com/aidantwoods/parsedown/blob/8a48dcfe315bc7e37028626646291538dce24d10/src/Components/Inlines/Image.php#L62
and used to generate the image (without directly depending on the HTML that the Link ends up producing).
This should hopefully allow a lot of reusability without binding so close to the internal behaviour that things like bugfixes can break dependents (as is the case at the moment).

Includes some general type safety related work (via static analysis).


Closes #440
Fixes #686
Closes #692
Fixes #104
Fixes #694

@aidantwoods aidantwoods added the WIP label Dec 5, 2018
@aidantwoods aidantwoods added this to the 2.0.0 milestone Dec 5, 2018
Don't use token name for function name

Remove return typehint

Remove parameter typehints
@aidantwoods
Copy link
Collaborator Author

cc @PhrozenByte if you'd be so kind as to review this :)

This was referenced Feb 21, 2019
composer.json Outdated Show resolved Hide resolved
@aidantwoods
Copy link
Collaborator Author

@PhrozenByte not sure if you had any time to take a look at this yet? No worries if not :)

@PhrozenByte
Copy link
Contributor

@PhrozenByte not sure if you had any time to take a look at this yet? No worries if not :)

Oh, sorry, completely missed your review request 😞 Will look into this as soon as possible, hope I'll find some time on Saturday.

@aidantwoods aidantwoods mentioned this pull request Mar 27, 2019
@aidantwoods
Copy link
Collaborator Author

aidantwoods commented Mar 27, 2019

Remaining progress:

I'm currently taking a look at implementing ParsedownExtra's features in terms of this new codebase—once I can do that I think this'll be pretty much ready to go (sans any fixes/concerns). I think I'll need to add something here to cope with abbreviation expansions (since they don't have a strict marker as such), my current line of thought for this a some concept of unmarked inlines can can be parsed before text is consumed as plaintext.

There is probably some room for improvement on the niceties of importing behaviour from an "extension" or many.

I'm not yet decided whether ParsedownExtra's features should live in core gated under optional non-default behaviour (as alluded to in #266 (comment)), or whether it should remain a separate extension. On the one hand, it's nice to have a single code-base (and people always seem to report ParsedownExtra issues here anyway), though it's also nice to have a reference extension.

@aidantwoods
Copy link
Collaborator Author

I'm going to merge this into the 2.0.x branch so further changes to this can be made in dedicated PRs (after over 200 commits here things are getting a little lost 😄). I'll open a separate PR to merge 2.0.x into master when changes are finalised. Please feel free to still add review comments to this PR though :)

@aidantwoods aidantwoods merged commit c8a37e7 into erusev:2.0.x Apr 2, 2019
@aidantwoods aidantwoods removed the WIP label Apr 2, 2019
@aidantwoods aidantwoods added this to In progress in 2.0.0 via automation Apr 8, 2019
@aidantwoods aidantwoods moved this from In progress to Done in 2.0.0 Apr 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
2.0.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants