feat: deprecation warnings on select application attributes#1116
feat: deprecation warnings on select application attributes#1116
Conversation
26be5d0 to
369eb96
Compare
james-garner-canonical
left a comment
There was a problem hiding this comment.
I had a couple of questions that I accidentally left "pending" all weekend, but thankfully Ben asked them today. Do you think it would be worth describing anything about Application's properties in the class docstring?
e4b9a8b to
1ec66d5
Compare
|
This package already imports typing_extensions via some dependency. I've made it an explicit dep, allowing for a simpler Here's a test run, showing correct source for the warning: /code/python-libjuju/example.py:28: DeprecationWarning: Application.owner_tag is deprecated and will be removed in v4
owner_tag.......... {app.owner_tag!r}
/code/python-libjuju/example.py:30: DeprecationWarning: Application.min_units is deprecated and will be removed in v4
min_units.......... {app.min_units!r}
/code/python-libjuju/example.py:32: DeprecationWarning: Application.subordinate is deprecated and will be removed in v4
subordinate........ {app.subordinate!r}
/code/python-libjuju/example.py:34: DeprecationWarning: Application.workload_version is deprecated and will be removed in v4, use Unit.workload_version instead.
workload_version... {app.workload_version!r} |
|
I see doc changes as orthogonal, so maybe in a separate PR? |
|
CI failures are unrelated, #1108 |
james-garner-canonical
left a comment
There was a problem hiding this comment.
The @deprecated decorator is cool, didn't know about that. One or two result in rather long lines, but I think we can look at that later if we introduce linting with ruff/etc.
The CI failures are all known failing tests. 3/5 fail 100% of the time on main.
test_wait_for_idle_more_units_than_needed fails 80% of the time on main.
test_subordinate_false_field_exists fails around 25% of the time on main.
The changes made in this PR seem reasonable to me, but I would like the source of truth for the new property names and their type annotations to be mentioned somewhere, if not in a docstring then perhaps somewhere it will show up in the git log.
"""Class representing a juju application.
Explicit properties are annotated with data types inferred from ...
See ... juju documentation page?
"""
I think I want this because I don't know where they come from or why they're correct, and maybe a future reader will wonder the same thing. But if you don't think this is worthwhile here then let's skip it for now.
Also, maybe we want a <5 restriction on the typing_extensions dependency just to be conservative? I know the existing dependencies are loosely specified, but that already caused at least one CI breakage.
1ec66d5 to
b359e0d
Compare
Done, ptal |
|
/build |
|
/merge |
186ab56 to
5de27a9
Compare
|
/merge |
Description