Skip to content

Update highlight markdown preprocessor to use register instead of add #6412

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

Closed
wants to merge 1 commit into from
Closed

Conversation

mcab
Copy link

@mcab mcab commented Jan 18, 2019

Description

refs #6317.

The issue mentioned that preprocessors from markdown now use register instead of add.

I followed the reference in the issue to see how preprocessors.register( is called, and updated the call accordingly. The only thing I'm unsure about is the priority, because add() had a call to place at the beginning, while register() relies on sorting. Since none go above 30, I believed 40 to be a safe bet.

I've ran tests (came back the same) and searched through docs, and I haven't seen this particular function mentioned, so, I'm unsure how to properly make sure this works.

Copy link
Collaborator

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Hi @mcab. Thanks for this!

Looks great, but do we need to adjust any markdown references in requirements and docs to specify > 3.0?

Also, we should add a note to the Release Notes.

@mcab
Copy link
Author

mcab commented Jan 22, 2019

Hey @carltongibson, glad to give a shot!

  • markdown updated their API for (pre)processors in version 2.6.11 (Python-Markdown/markdown@2.6.11...master), which requirements/requirements-optional.txt already specifies. So, I don't believe we need to version bump for requirements or docs.

  • Release Notes wise, I think this would fall under the next minor release, but I'm unsure of the protocol for updating such.

@carltongibson
Copy link
Collaborator

carltongibson commented Jan 24, 2019

Hi @mcab. I think the registry was added in 3.0. Can you double check? Ta.

https://github.com/Python-Markdown/markdown/blob/master/docs/change_log/release-3.0.md

Given that was only last-Qtr: were there breaking changes, beyond deprecations? If so, we need to go slower. If not, no harm asking people to update.

I added a release note stub for v3.9.2. f539c0d If you could add a note there, that would be super!

Thanks for the effort. 🎁

@mcab
Copy link
Author

mcab commented Jan 28, 2019

Hey @carltongibson, internet was out for a few days, but I'm back in action now.

Registry changes ended up in the 3.0 markdown release, so I've bumped up requirements accordingly. I've ran tox using my changes and bumping the version of markdown to 2.6.11, 3.0, and 3.0.1, and there were no differences in tests successfully run / failed.

I'll be doing a final run against the current master branch to ensure there wasn't any other issue with bumping up the version requirement of markdown to 3.0.1.

I've also added the changes under the release note stub.

With this change, Markdown version requirement can also be bumped up to
3.0.1 to use .register() instead of .add().
@carltongibson carltongibson added this to the 3.10 Release milestone Feb 14, 2019
@carltongibson
Copy link
Collaborator

Hi @mcab. Just to let you know, I'm think we should just hold this until 3.10, just so as not to push a break onto anybody without due warning.

I'm quite sure that if we do this nobody had a breakage, where if we stick it in 3.9.2 somebody will scream. 🙂

@rpkilby
Copy link
Member

rpkilby commented May 31, 2019

Hi @mcab. I've updated this in #6722, which resolves a few merge conflicts and makes a few additional changes.

@rpkilby rpkilby closed this May 31, 2019
@rpkilby rpkilby removed this from the 3.10 Release milestone May 31, 2019
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.

3 participants