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

First version for options in a dict #82

Closed
wants to merge 12 commits into from
Closed

Conversation

rokdd
Copy link
Contributor

@rokdd rokdd commented Jun 14, 2021

First version for a dict handling the options. No error handling if the wrong type is given. Planned a structure like:

WAGTAILMARKDOWN = { 
#this I would not name with bleach.. 
'allowed_tags': [],
...
extensions; {},
extensions_config: {},
..
}

First version for a dict handling the options.
@zerolab
Copy link
Member

zerolab commented Jun 15, 2021

Hey @RoKondo, thanks for this.
It is along the lines I was thinking of, so you're up to a great start

To finish off, this PR needs:

  • updating the docs (well, the README)
  • to be a bit more tidy up (e.g. allowed_styles, allowed_tags are lists which when added may result in duplicates. probably ok for bleach, but a nice thing to have)
  • cater for the WAGTAILMARKDOWN_AUTODOWNLOAD_FONTAWESOME setting (https://github.com/torchbox/wagtail-markdown/blob/main/wagtailmarkdown/widgets.py#L22)
  • and maybe raising deprecation warnings for the separate settings so that people can update to the new format.

@rokdd
Copy link
Contributor Author

rokdd commented Jun 15, 2021

Well nope,will do it tomorrow. With the last one I thought to still accept the old prefs. But obviously you agree to my concept of the options.

Added warning for deprecation
Added deprecation warning
Added better merging of list to avoid duplicates.
wagtailmarkdown/utils.py Outdated Show resolved Hide resolved
Added the new configuration with dict WAGTAILMARKDOWN
@rokdd
Copy link
Contributor Author

rokdd commented Jun 16, 2021

I changed the readme also a bit by structure.. I am finished from my side.

Copy link
Member

@tomdyson tomdyson left a comment

Choose a reason for hiding this comment

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

Two tiny tweaks. Elsewhere, the quoting style is inconsistent in Python code. Can we use black from now on?

README.md Outdated Show resolved Hide resolved
wagtailmarkdown/utils.py Outdated Show resolved Hide resolved
rokdd and others added 2 commits June 16, 2021 11:15
Co-authored-by: Tom Dyson <tom@torchbox.com>
Co-authored-by: Tom Dyson <tom@torchbox.com>
@rokdd
Copy link
Contributor Author

rokdd commented Jun 16, 2021

Two tiny tweaks. Elsewhere, the quoting style is inconsistent in Python code. Can we use black from now on?

Thanks.. shall I change to black ?

@zerolab
Copy link
Member

zerolab commented Jun 16, 2021

Thank you both,
we have pre-commit support - https://github.com/torchbox/wagtail-markdown/blob/main/.pre-commit-config.yaml

best to install pre-commit locally:

$ pip install pre-commit
# in project dir
$ pre-commit install
# run all checks once for this, then the checks will run only on the changed files
$ pre-commit run --all-files

@rokdd
Copy link
Contributor Author

rokdd commented Jun 16, 2021

Thanks for the pre-commit tool - pretty cool. Hope it is fine now.

@zerolab
Copy link
Member

zerolab commented Jun 18, 2021

Thank you for this PR @rokdd
Tidied it up and merged in 99f6cd6

While doing the tidy up, I realised WAGTAILMARKDOWN_EXTENSIONS, WAGTAILMARKDOWN_EXTENSIONS_CONFIG and WAGTAILMARKDOWN_AUTODOWNLOAD_FONTAWESOME were all in a pre-release so decided it is cleaner to do away with the deprecation warnings. Sorry for the extra work!

@zerolab zerolab closed this Jun 18, 2021
@zerolab zerolab mentioned this pull request Jun 19, 2021
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.

None yet

3 participants