DM-39227: Add support for deprecating connections. #349
Conversation
TallJimbo
force-pushed
the
tickets/DM-39227
branch
from
fd13f9f
to
c2187a6
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #349 +/- ##
==========================================
+ Coverage 82.72% 82.81% +0.09%
==========================================
Files 66 66
Lines 7246 7252 +6
Branches 1408 1411 +3
==========================================
+ Hits 5994 6006 +12
+ Misses 1005 1000 -5
+ Partials 247 246 -1
☔ View full report in Codecov by Sentry. |
| @@ -57,6 +57,14 @@ class BaseConnection: | |||
| doc: str = "" | |||
| multiple: bool = False | |||
|
|
|||
| deprecated: str | None = dataclasses.field(default=None, kw_only=True) | |||
| """A description of why this connection is deprecated, including the | |||
There was a problem hiding this comment.
It seems a bug that we are not documenting doc above. It seems weird that the docs for this are here and not up with the docstring like the others. What does sphinx perfer? I know python does nothing with strings defined below an attribute and just drops them on the floor, so interactively the docs are unavailable. Can you make these docs consistent one way or another?
There was a problem hiding this comment.
Ah, I hadn't even noticed that the others had been documented earlier. I usually think of the class doc's "Parameters" section as being for __init__ arguments, not instance attributes, but I suppose for dataclasses there's (usually) not much distinction. I'll just move this docstring up with the rest.
There was a problem hiding this comment.
In a dataclass they are all init args unless you play with that though right? Though I think that is a bit of a pain because of playing with the sense of args.
There was a problem hiding this comment.
Like I said, I am fine if you want to move them all there, but it is a bit sad because it wont show up with help
There was a problem hiding this comment.
Right. My tendency is to add class Parameters to a dataclass only if I actually define an __init__ but to always document the attributes. I don't think that's obviously better or worse than only documenting them in class Parameters as you've done here, though maybe we should always be documenting both? (Of course, duplication is then its own problem.)
There was a problem hiding this comment.
I have actually moved the new one to Parameters; I was just explaining my usual practice elsewhere. I'd rather resolve the general question about which (if either) of us should adopt the other's practice outside this ticket, and just keep things closer to the way they are already are for now.
TallJimbo
force-pushed
the
tickets/DM-39227
branch
from
786867b
to
fff16a1
Compare
I was originally thinking that'd have to be an unconditional warning, which would be bad, but I think I could make it warn if any deprecated connections have not been removed by the end of the Connections construction. I'll give that a try. |
TallJimbo
force-pushed
the
tickets/DM-39227
branch
from
2611742
to
641a18e
Compare
This intentionally just deprecates the corresponding config entry, keeping those config fields from being written. The expectation is that deprecated templates may be used in also-deprecated connections without emitting another warning, and then both can be removed together.
TallJimbo
force-pushed
the
tickets/DM-39227
branch
from
641a18e
to
8b58026
Compare
|
@andy-slac , I just realized that @natelust is out for a few days and hence won't see my previous comment about the last two commits here. Would you mind filling in as reviewer for those? |
|
Since the rest of this ticket has a longer test-and-review future ahead of it and it's logically distinct, I've moved the changes on this branch to DM-39902 and #351. |
Checklist
doc/changes