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: Report usage-based warnings as user-facing messages #464

Merged
merged 4 commits into from Aug 20, 2022

Conversation

oprypin
Copy link
Member

@oprypin oprypin commented Aug 16, 2022

The other warnings are developer-facing and should indeed remain as DeprecationWarning.

@oprypin oprypin changed the title Report usage-based warnings as user-facing messages refactor: Report usage-based warnings as user-facing messages Aug 16, 2022
The other warnings are developer-facing and should indeed remain as DeprecationWarning.
@pawamoy
Copy link
Member

pawamoy commented Aug 17, 2022

So the rationale is that docs writers are not always developers and won't care about Python warnings? Or even have them disabled? I'm not sure they would care more about an INFO log... 🤔

@oprypin
Copy link
Member Author

oprypin commented Aug 17, 2022

  1. I think users would care more about an info log compared to literally nothing as it is now - as indeed almost nobody enables warnings.
  2. DeprecationWarning means very simply "don't call this function like that". People aren't calling any functions here. There's nothing to learn from a stack trace. Indeed, why is mkdocstrings calling its own deprecated function? That's what DeprecationWarning is saying

@oprypin
Copy link
Member Author

oprypin commented Aug 17, 2022

Yet another way to look at it is:
DeprecationWarning means - upgrade everything to latest and if the issue still persists, complain to the developer of the plugin. These cases that I'm changing are incorrectly suggesting this.

@pawamoy
Copy link
Member

pawamoy commented Aug 17, 2022

To me a deprecation warning is not exclusively used for functions. We deprecated using selection and rendering keys, so we inform the users with the standard deprecation mecanisms that they should stop using them, because they will disappear in the next versions and things will break. Users/devs who care about keeping up-to-date dependencies should enable warnings, as best-practice. If they don't and get caught by a breaking change later, that's on them 😕 Though I agree an additional log cannot hurt.

@oprypin
Copy link
Member Author

oprypin commented Aug 17, 2022

Do you know any application that informs its users about removing features by emitting a Python DeprecationWarning (which, again, almost nobody will see)?
The fact that this feature removal happens to be implemented by reorganizing some Python code is completely coincidental. Users don't care to see references to Python.
I don't know why you call it a "standard mechanism" in this context when its only purpose is to tell developers to change their Python code in response to some other change. It actually directly says so here https://docs.python.org/3/library/exceptions.html#DeprecationWarning

@oprypin
Copy link
Member Author

oprypin commented Aug 17, 2022

Hmm but maybe we could consider just integrating this kind of warning instead https://docs.python.org/3/library/exceptions.html#UserWarning - it even seems to be enabled by default

@oprypin
Copy link
Member Author

oprypin commented Aug 17, 2022

Well, it still talks about "user code" so still not a great fit

@oprypin
Copy link
Member Author

oprypin commented Aug 17, 2022

https://docs.python.org/3/howto/logging.html#when-to-use-logging also confirms that the expected action in response to Warnings is to change the code of the application. Which doesn't match in these cases that I'm suggesting to change.
I also plan to make this the official guidance of MkDocs.


And sorry that I sound so aggressive about this 😞

@oprypin
Copy link
Member Author

oprypin commented Aug 20, 2022

@pawamoy
Copy link
Member

pawamoy commented Aug 20, 2022

Thanks for the links, it seems the DeprecationWarning should not be used for that indeed. How do you feel about prefixing the log line with "DEPRECATION"? I worry users will simply miss/ignore them.

@oprypin
Copy link
Member Author

oprypin commented Aug 20, 2022

OK! Why not.

Here's how it will look (first warning is unrelated but nice to see in surrounding)

INFO     -  DeprecationWarning: 'UnescapePostprocessor' is deprecated. This class will be removed in the future; use 'treeprocessors.UnescapeTreeprocessor' instead.
              File "/home/blaxpirit/.local/lib/py/mkdocs_literate_nav/parser.py", line 22, in <module>
                _unescape = markdown.postprocessors.UnescapePostprocessor().run
              File "/home/blaxpirit/.local/lib/py/markdown/util.py", line 103, in deprecated_func
                warnings.warn(
INFO     -  mkdocstrings: DEPRECATION: mkdocstrings' watch feature is deprecated in favor of MkDocs' watch feature, see https://www.mkdocs.org/user-guide/configuration/#watch
INFO     -  Cleaning site directory
INFO     -  mkdocstrings: DEPRECATION: 'selection' and 'rendering' are deprecated and merged into a single 'options' YAML key
INFO     -  Documentation built in 0.41 seconds
INFO     -  [12:36:10] Watching paths for changes: 'docs', 'mkdocs.yml', 'mkdocs_gen_files'

Copy link
Member

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

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

Thanks!

@pawamoy pawamoy merged commit 03dd7a6 into mkdocstrings:master Aug 20, 2022
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

2 participants