Added docstring and type annotations to family(), removed parameter endpoint #87
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #87 +/- ##
===========================================
- Coverage 99.76% 77.54% -22.23%
===========================================
Files 18 18
Lines 428 432 +4
===========================================
- Hits 427 335 -92
- Misses 1 97 +96
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
epo_ops/api.py
Outdated
| def family(self, reference_type, input, endpoint=None, constituents=None): | ||
| def family( | ||
| self, | ||
| reference_type: str, | ||
| input: Union[Docdb, Epodoc], | ||
| constituents: Optional[List[str]] = None, | ||
| ): |
There was a problem hiding this comment.
Acc. to ops specs, endpoint is not used in family service. Not used in tests either. Options: either document it in docstring or remove it. Went for second option.
Removing parameters means a breaking change, so we would need to target this for a 5.x release. Is it possible to move on with type hinting without doing that, and saving it for a subsequent iteration?
There was a problem hiding this comment.
In order to prepare the removal, we can issue a deprecation warning when it is used, like:
if endpoint is not None:
warnings.warn("The `endpoint` argument is not used in this context and will be removed.", DeprecationWarning)There was a problem hiding this comment.
You are quite right, I was a bit too fast on this one. Thanks.
| endpoint=endpoint, | ||
| endpoint=None, |
There was a problem hiding this comment.
If we are 100% sure on this, let's do it. Otherwise, it would also be a breaking change, if, by chance, the parameter would still be used/needed in some cases.
There was a problem hiding this comment.
I tested the family() method, and the implementation allows endpoint to be assigned one of the allowed values for the parameter constituents and still work as if constituents was assigned the value. Although not according to ops specs, the resulting url when using e.g. endpoint="biblio" is the same as if constituents=["biblio"] was used.
So removing it may indeed break code out there. I'd leave that for a major version upgrade. Fine with the deprecation warning.
There was a problem hiding this comment.
Hi Matt. Apologies for the delay.
So removing it may indeed break code out there. I'd leave that for a major version upgrade. Fine with the deprecation warning.
I am currently not totally sure, it has been a while... Do you agree that we want to keep endpoint=endpoint? It looks like the patch hasn't changed yet.
On the other hand, I don't want to be too pedantic about it if you think it will be good to go.
| endpoint=endpoint, | ||
| endpoint=None, |
There was a problem hiding this comment.
Hi Matt. Apologies for the delay.
So removing it may indeed break code out there. I'd leave that for a major version upgrade. Fine with the deprecation warning.
I am currently not totally sure, it has been a while... Do you agree that we want to keep endpoint=endpoint? It looks like the patch hasn't changed yet.
On the other hand, I don't want to be too pedantic about it if you think it will be good to go.
|
Would it help if I check out this branch and use it in my tests? |
@CholoTook would be great, yes. |
Starting with type annotations.
Acc. to ops specs, endpoint is not used in family service. Not used in tests either. Options: either document it in docstring or remove it. Went for second option.
The type annotations are acc. to python 3.6 (although commit comment says py3.8).