Allow custom escape

[CarliJoy]

• fixes #1377

Checklist:

• [x] Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
• [x] Add or update relevant docs, in the docs folder and in code.
• [x] Add an entry in CHANGES.rst summarizing the change and linking to the issue.
• [x] Add .. versionchanged:: entries in any relevant code docs.
• [x] Run pre-commit hooks and fix any issues.
• [x] Run pytest and tox, no tests failed.
• [x] Check all left TODOs
  • [x] Make sure that the left Markup() calls do not overwrite custom escapes with HTML escapes
  • [x] if possible create yet some more test for some changes (see TODOs as well)
• [x] add some more changed version documention, but IMO low prio
• [x] add a test that uses files that have includes / extends templates that have different endings (configured to different auto escapern)

[CarliJoy]

Hi guys,

thanks for your efforts, will work you comments in tomorrow or the days after tomorrow. Actually PyCharm has Spell Check enabled but especially for grammar stuff it's not that good yet. Probably I also have to change some settings as I guess I just missed some spell underlines 🤷🏻 🤦🏻 As I wrote in #1377, I am bit of (maybe not only a bit ;-) ) dyslexic, even in my native language (the one I probably share with @jugmac00).

So thanks for taking the effort in checking and correcting -> much appreciated!

Also is the first time for me that an PR in github actually got reviewed, so I also need to get started on this slowly :) And please be forgiveful for my mistakes :-)

[CarliJoy]

@davidism is okay for your to take care of the conflict in docs/api.rst that was introduced through: #1391?

[CarliJoy]

the moment when you worked 4hours to resolve a merge conflict and you realise that in the meanwhile the next PR was merged that causes the next merge conflict 😭

[CarliJoy]

@ThiefMaster I fixed everything so far. Documentation is up to date, tests are as well. Currently no Merge Conflicts. How likely is that you will give your ok for the PR? Because I rather would not to spend any more work into something that goes into the bin.

Also because this change touches code on so many places, that other PR most likely will introduce other Merge Conflicts.

[ThiefMaster]

Because I rather would not to spend any more work into something that goes into the bin.

Don't worry about that. If we believed this was something we don't want we'd have told you from the beginning.

However, this is a large and complex PR, and while I will have another look, I don't think I'm going to be the one that will take the final decision on it and merge it; I'll leave that up to @davidism once he has the time for it.

I wouldn't worry too much about merge conflicts - most of them can probably be resolved right before merging.

[CarliJoy]

@davidism once you check the code, have a lock especially on the async stuff. I have no experience with async coding what so ever, so my changes are more like an educated guess. I still tried to write some tests but not sure if I covered all the cases. Maybe you also find a way make EvalContext known to the async Macros....

[CarliJoy]

@davidism can I support/help you in any way with the review?

[davidism]

We won't have time to review something of this magnitude before release, so we're marking this for 3.1. I'm currently adding type annotations to the entire code base, so this is likely to need a rebase after that. Rebasing in general to squash commits would also be helpful, as 71 is too many to review.

[CarliJoy]

Sorry I am not yet that experienced with rebasing. What would be the workflow in an existing PR like this? I would assume:

1. After your added the Type Annotations create a new branch like custom_escape_squashed base on main
2. Merge squash the existing custom_escape in the new branch
3. Create a new PR based on the squashed branch and close this PR

Would that work/help? Or what else do you mean?

[jugmac00]

I think @davidism meant, that you should squash (https://levelup.gitconnected.com/how-to-squash-git-commits-9a095c1bc1fc) your 71 commits into one (and maybe force push the result to this branch).

After @davidism applied the type annotations, and merged them into master/main, you can rebase onto master/main - but only this one commit (https://stackoverflow.com/questions/7929369) - which makes it much easier.

Finally, force-push to this branch.

So, it is neither required to create a new branch or a new PR.

[davidism]

Hold off on squashing until I merge the type annotations. It might actually be easier to recreate after the fact instead of trying to resolve the merge conflicts, which will be significant.

[CarliJoy]

@davidism just tell me once you finished the merge of the type annotations...

[CarliJoy]

I think @davidism meant, that you should squash (https://levelup.gitconnected.com/how-to-squash-git-commits-9a095c1bc1fc) your 71 commits into one (and maybe force push the result to this branch).

After @davidism applied the type annotations, and merged them into master/main, you can rebase onto master/main - but only this one commit (https://stackoverflow.com/questions/7929369) - which makes it much easier.

Finally, force-push to this branch.

So, it is neither required to create a new branch or a new PR.

Thanks for the detailed answer

[davidism]

They're merged

[CarliJoy]

@davidism Did the rebase and combination as requested. One thing: When are you planning to release 3.0? Are you following SemVer? Because if you do it would be good not only to deprecate Markup but directly make people aware of the functions that will return the correct Markdown class? Otherwise there will be one braking change between 3.0 and 3.1.

My suggestion would be to create a small PR that only provides the functions to get the (then only hardcoded) Markdown and escape functions.

Is this understandable? What do you think? Okay just realized that you release 3.0 already, well then there will be a braking change between 3.0 and 3.1...

[davidism]

We don't follow semver.

I don't understand what the breaking change is, the only thing this PR should be doing is adding customization capabilities, not changing existing behavior.

[CarliJoy]

First of all. Sorry I missed that release. Big thing, updating all libraries at once. Congratulations 🎉

We don't follow semver. Good to know.

I don't understand what the breaking change is, the only thing this PR should be doing is adding customization capabilities, not changing existing behavior.

Braking change is for all creators of custom extensions and people that use Markdown directly in normal Python code to define a string as safe. If they combine it with custom escape it could lead to unexpected behaviour (i.e. when using the % operator). Tried to explain that in the documentation of the PR as good as possible.

[davidism]

That's not a breaking change though, that use case will continue to work, but they'll need to update if they want to take advantage / make correct use of customization that this enables. That's what a good changelog message and docs are for, so if you've addressed that there, that's good.

[CarliJoy]

@davidism I changed things as requested. If there are still any issues, let me know.

[CarliJoy]

@davidism and @ThiefMaster will this ever be merged?

[davidism]

Yes, I do plan to eventually merge this. It is a huge set of changes, and I just have not been able to sit down and properly review it.

[CarliJoy]

@davidism just another ping. Anything I can do to support you on this MR?

[CarliJoy]

@davidism And here the yearly ping again 😉 . Anything I could do to finally finally get this merged?