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 em strong to consolidate code and fix issue #792 #805

Merged
merged 9 commits into from
Sep 3, 2019
Merged

Refactor em strong to consolidate code and fix issue #792 #805

merged 9 commits into from
Sep 3, 2019

Conversation

facelessuser
Copy link
Collaborator

This refactors em strong logic into two processors: one for asterisks and one for underscores. All logic for the respective emphasis convention is consolidated, and a new pattern is added to handle **strong*em*** and __strong_em___ respectively.

@facelessuser facelessuser added the work-in-progress A partial solution. More changes will be coming. label Mar 13, 2019
@facelessuser
Copy link
Collaborator Author

Tests are still needed for the new, fixed case.

@facelessuser
Copy link
Collaborator Author

Current tests are now passing, but will have to address coverage as well.

markdown/inlinepatterns.py Outdated Show resolved Hide resolved
@facelessuser
Copy link
Collaborator Author

PyYaml 5.1 wrecking Python 2 tests 😡 .

@waylan
Copy link
Member

waylan commented Mar 13, 2019

PyYaml 5.1 wrecking Python 2 tests.

That is annoying. I understand why PyYAML made the changes they did, but we were relying on the fact that the PyYAML and json libs both used the same API and PyYAML could parse JSON. We also avoided requiring PyYAML as a hard dependency as we used the json lib as a fallback. Now we need a more complicated implementation to account for the differences between the libs' APIs.

@facelessuser
Copy link
Collaborator Author

Do you know why it is only failing on Python 2?

@waylan
Copy link
Member

waylan commented Mar 13, 2019

Do you know why it is only failing on Python 2?

I do not. The lowdown on the changes are here. I suppose we could simply disable the warning as an easy fix. We only use the YAML loader in the CLI so if there is a vulnerability there, the user has much worse problems that arbitrary code injected in a YAML config file.

@waylan
Copy link
Member

waylan commented Mar 14, 2019

The PyYAML issue has been addressed in #806. Feel free to rebase your work on master now.

@waylan waylan mentioned this pull request Mar 26, 2019
@waylan waylan added this to the Version 3.2 milestone Mar 26, 2019
@facelessuser
Copy link
Collaborator Author

Ugh, everything broke for some reason... Probably need to rebase maybe

@facelessuser
Copy link
Collaborator Author

Tests appear to be broken due to the latest tox 3.8+. By forcing 3.7 tox, tests begin passing again.

@facelessuser
Copy link
Collaborator Author

Maybe related to tox-dev/tox#1236.

@waylan
Copy link
Member

waylan commented Mar 31, 2019

Let's see what tox does before we address this. If they fix it quickly in the next release, then we might not need to do anything.

@facelessuser
Copy link
Collaborator Author

Yeah, I'm obviously in no hurry as I've been taking my time. The current work around was just to identify what I figured was the problem, it isn't meant to be the official solution unless we are desperate.

@facelessuser
Copy link
Collaborator Author

Okay, I finally got around to rebasing this. Obviously tox is no longer broken, so I reverted the workaround. I'm pretty much done with the work here unless there are further requests. In the future, maybe the NOT_STRONG_RE logic could be incorporated into this, but that would really require rewriting some of the existing patterns which I'm not looking to do at this time.

@facelessuser facelessuser added needs-review Needs to be reviewed and/or approved. and removed work-in-progress A partial solution. More changes will be coming. labels Sep 1, 2019
@waylan
Copy link
Member

waylan commented Sep 1, 2019

Look good. Just needs a note in the release notes. Feel free to added a new release-3.2.md file.

@waylan
Copy link
Member

waylan commented Sep 2, 2019

As we are removing many existing inline processors and replacing them with two new ones, I think we probably need to make mention of that in the release notes. For example, if an extension is removing all of the existing inline processors, the dev will need to do some work to make it compatible. The release notes needs to call this out. That's why I suggested 3.2 rather than 3.1.1 for the release.

@facelessuser
Copy link
Collaborator Author

Makes sense. I'll adjust within the next day or two.

@waylan waylan merged commit ce04b81 into Python-Markdown:master Sep 3, 2019
@facelessuser facelessuser mentioned this pull request Sep 4, 2019
@waylan waylan mentioned this pull request Feb 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-review Needs to be reviewed and/or approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants