CU-86948uv4g docstring signature consistency #413
Conversation
…ures match doc strings)
…ures match doc strings)
… signatures match doc strings)
|
Task linked: CU-86948uv4g Ensure docstrings match signature |
| @@ -604,7 +616,7 @@ def unlink_concept_name(self, cui: str, name: str, preprocessed_name: bool = Fal | |||
|
|
|||
| cuis = [cui] | |||
| if preprocessed_name: | |||
| names = {name: 'nothing'} | |||
| names = {name: {'nothing': 'nothing'}} | |||
There was a problem hiding this comment.
interesting - so this was a bug before?
There was a problem hiding this comment.
The issue had to do with later calling CDB.remove_names with the names argument.
The names could either have the type Dict[str, Set[str]] (if preprocessed_name) or Dict[str, Dict] otherwise from medcat.preprocessing.cleaners.prepare_name (although the type hasn't been explicitly type-hinted by the method return type).
Before I started this procedure, the type hint for the CDB.remove_names method's names argument was simply Dict. So that worked with either of the aforementioned inputs.
However, the doc string said Dict[str, Dict]. So since flake8 was now checking doc strings, I assumed the more restrictive type would be appropriate and changed the type hint for the method.
After having made all the doc string changes, mypy was now having issues for the argument type. Which is why I made this change.
With that said, the CDB.remove_names method only acts on the keys of this dict. So it doesn't really matter what the values are. It's just that in most cases, the results from medcat.preprocessing.cleaners.prepare_name would be expected to be provided.
So all in all not necessarily a bug. Behaviour isn't changed, after all. But rather a clarification of the types of data generally expected.
EDIT:
Though in retrospect, we may want to just accept any Iterable[str] for CDB.remove_names and just iterate over it. Clearly it doesn't use the values so we don't really need to restrict the input to dicts.
There was a problem hiding this comment.
yeah that makes more sense . slightly less confusing
| @@ -604,7 +616,7 @@ def unlink_concept_name(self, cui: str, name: str, preprocessed_name: bool = Fal | |||
|
|
|||
| cuis = [cui] | |||
| if preprocessed_name: | |||
| names = {name: 'nothing'} | |||
| names = {name: {'nothing': 'nothing'}} | |||
There was a problem hiding this comment.
yeah that makes more sense . slightly less confusing
Many people work on this project. And while we try our best to do reviews on PRs, not everything is always noticed. Especially with respect to documentation.
So when there's a change / PR for a method, sometimes the method's signature changes. But sometimes that change isn't reflected in the actual documentation (doc string).
So this PR attempts to automate the validation of doc strings to make sure they match the actual method signature they're trying to describe.
The way it does that is this:
flake8==7.0.0for linting4.0.1beforeWhile looking for a solution for this, I also looked at a few other tools. But it felt easier / better to stick to what we have and update that.
With that said, due to the many-many new checks the new
flake8version provides, there were many minor changes - mostly to doc strings and/or method signature type hints - in many modules.If we merge this PR, going forward,
flake8will always run as part of GitHub Actions workflow (as it has so far). With the caveat that it will now catch any discrepancies in doc strings and method bodies as well.