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

Refactor Markdown Extension Options Config. #528

Merged
merged 2 commits into from
May 16, 2015
Merged

Conversation

waylan
Copy link
Member

@waylan waylan commented May 15, 2015

Config validation now handles all extension processing:

  • Builtin extensions are defined and within the default scheme.
  • User extensions are defined only as a list in the config. Note, this is
    a backward incompatable change from the previous (short-lived) release.
  • The users extensions are added to the list of builtins.
  • Any duplicates are accounted for in validation.
  • Extension options are supported by a child key/value pair on the ext name.
  • All extension options are compiled into a format Markdown accepts
    within the validation process and are saved to the internal mdx_configs
    config setting.
  • The mdx_configs setting is private and raises an error if set by the user.
  • A whole suite of tests were added to test all aspects of ext validation.

All relevant build tests were updated to pass the config to
build.convert_markdown as the config now handles all extension data.

The relevant documentation was updated to reflect the changes. While I was
at it, I spellchecked the entire document and made a few additional formatting
changes.

This fixes #519 plus a lot more.

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling d3fa1bd on waylan:519 into 667fc66 on mkdocs:master.

@d0ugal
Copy link
Member

d0ugal commented May 15, 2015

This looks great! I should be able to review it today.

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling 06a74af on waylan:519 into 93a181a on mkdocs:master.


def run_validation(self, value):
if isinstance(value, dict):
# Delete this if block when dict support is removed
Copy link
Member

Choose a reason for hiding this comment

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

I've been trying to add # TODO: remove in 1.0 to these. It could happen sooner, potentially, but 1.0 would be the latest this should happen.

@d0ugal
Copy link
Member

d0ugal commented May 16, 2015

This is lovely, do you plan to do any more updates? I'm happy to merge as it is. Tested it and it works great.

@waylan
Copy link
Member Author

waylan commented May 16, 2015

The only other thing I would consider doing at this point is removing the OrderedDict stuff for the config as this removes the need for it. I don't think it would hurt to leave it in, but removing it removes one more thing to maintain long-term. @facelessuser have any thoughts?

Regardless, that could happen latter. Feel free to merge any time.

@facelessuser
Copy link
Contributor

Remove it, unless ordered dicts will be desired somewhere in the future. It hurts nothing, but I only added it because, at the time, it was decided not to do the extension config this way. But now we are, so the original need is gone.

Config validation now handles all extension processing:

* Builtin extensions are defined and within the default scheme.
* User extensions are defined only as a list in the config. Note, this is
  a backward incompatable change from the previous (short-lived) release.
* The users extensions are added to the list of builtins.
* Any duplicates are accounted for in validation.
* Extension options are supported by a child key/value pair on the ext name.
* All extension options are compiled into a format Markdown accepts
  within the validation process and are saved to the internal `mdx_configs`
  config setting.
* The `mdx_configs` setting is private and raises an error if set by the user.
* A whole suite of tests were added to test all aspects of ext validation.

All relevant build tests were updated to pass the config to
`build.convert_markdown` as the config now handles all extension data.

The relevant documentation was updated to reflect the changes. While I was
at it, I spellchecked the entire document and made a few additional formatting
changes.

This fixes mkdocs#519 plus a lot more.
@waylan
Copy link
Member Author

waylan commented May 16, 2015

On further thought, the OrderedDict stuff should be removed with the support for a dict for markdown_extensions. I'm suggesting we leave it in for now. I just pushed an update with TODOs at the relevant parts of the code (also swashed a few commits for a cleaner history). I'm done with this. Feel free to merge anytime.

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling 27291a0 on waylan:519 into 93a181a on mkdocs:master.

@facelessuser
Copy link
Contributor

On further thought, the OrderedDict stuff should be removed with the support for a dict for markdown_extensions.

Good point, I checked that you were preserving the new list order with reduce_list, but I forgot to check if you were still maintaining the old dict format. Technically, a release with ordered dicts hasn't been released yet, so I wonder if you really need to keep it. It was scheduled for 0.13, but that still hasn't dropped yet.

With the markdown_extensions refactor, all extensions are defined in a list
which retains order. No need for an Ordered Dict. As the OrderedDict stuff
was never released, no deprecation path is nessecary.
@waylan
Copy link
Member Author

waylan commented May 16, 2015

Technically, a release with ordered dicts hasn't been released yet, so I wonder if you really need to keep it. It was scheduled for 0.13, but that still hasn't dropped yet.

Ok, I was thinking a release had happened, but you are right it didn't. I just ripped it out. I really am done now.

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling 7cd2ef5 on waylan:519 into 93a181a on mkdocs:master.

@d0ugal
Copy link
Member

d0ugal commented May 16, 2015

Thanks! Just waiting for CI and then I'll merge.

d0ugal added a commit that referenced this pull request May 16, 2015
Refactor Markdown Extension Options Config.
@d0ugal d0ugal merged commit a47dfbc into mkdocs:master May 16, 2015
@waylan waylan deleted the 519 branch June 2, 2015 13:30
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.

smartypants extension error
4 participants