-
-
Notifications
You must be signed in to change notification settings - Fork 355
Add option to change title format #842
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
base: master
Are you sure you want to change the base?
Conversation
thp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cfg.get() call with a default is great for existing configurations that lack that entry.
New configs should however have title: default in there, this makes it also a bit easier to discover this feature (add it to the relevant entries of DEFAULT_CONFIG in storage.py).
Since the gotify reporter already uses title, and we might want to use title for something else in the future, AND you already call it title_format in the Python variable that gets used (for good reason: it's the title format, not the title itself), I'd suggest we call it title_format in the config as well.
Also, there should be added documentation for this feature, so that users can find out about how it works and what possible values exist for title_format and to which reporters it applies.
Another thing I would do to detect misconfigurations (instead of accepting them silently):
if title_format not in ("default", "location", "pretty"):
logger.warn(f'Invalid title format {title_format!r}, falling back to "default".')
title_format = "default"Otherwise, if I set title_format: foo, it will happily accept this without complaint, and use the default format. If a future version adds e.g. "fancy" as a title format, and someone tries to use it, but uses a version that doesn't have that title format supported yet, instead of silently falling back to "default", it will still fall back to "default" (a sensible behaviour), but also write out a warning, giving a clue to the user that the program has done the fallback, because it doesn't recognize the value set.
|
I added the default options and the warning. |
|
@thp I added the format options to the places I mentioned. If you would like more elaborate documentation of the title_format feature, let me know where in the docs that would make the most sense. |
Probably a section in the https://github.com/thp/urlwatch/blob/master/docs/source/advanced.rst page, something like "Customizing the title format in reports", with all available options and examples what they do. |
@E1k3 Adding the docs would make this PR merge-ready, I believe. |
This PR adds the option to change the report title format as discussed in #385.
I added the option to HtmlReporter, TextReporter and MarkdownReporter, because all of these currently use the "pretty (location)" default format in some way.
I tested the text and markdown reporters and they work like discussed. I couldn't test the html reporter, because I couldn't get my mailserver to accept my smtp password for some reason.
For now, every setting other than "pretty" and "location" falls back to the default format.
If you want an error message for unknown title format specifiers instead, let me know.