fix: len(None) bug #643
fix: len(None) bug #643
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: codecov_cli/services/upload/upload_collector.py
Did you find this useful? React with a 👍 or 👎 |
❌ 5 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
|
✅ All tests successful. No failed tests were found. 📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
This is an upstream bug that is being fixed. This patch will have to be reverted. Refs: * codecov/codecov-cli#641 * codecov/codecov-cli#643
| self.network_prefix = network_prefix | ||
| self.network_root_folder = network_root_folder | ||
|
|
||
| def find_files(self, ignore_filters=False) -> typing.List[str]: |
There was a problem hiding this comment.
Is mypy run on this project?
If files is None (as described in the PR description), your changed code surely results in returning None, in which case this annotation is wrong and there's a good chance another part of the codebase could mess up when expecting a list.
There was a problem hiding this comment.
OK, I see the root problem (which confirms to me that this is not being checked by mypy, as it should have errored here).
The method is defined on the base class, but it's not an abstract method. So, if inherited (or instantiated directly), this returns None, even though the annotation says it returns a list (which is a clear type error that would be caught by any type checker):
codecov-cli/codecov_cli/helpers/versioning_systems.py
Lines 23 to 24 in 63bd5f2
I think the correct fix is to either replace that pass with a return [], so it actually does the correct behaviour, or to turn those methods into abstract methods and require the subclass to define them correctly.
There was a problem hiding this comment.
Thanks for the feedback, I've implemented the ABC / abstractmethod suggestion.
the bug was the the NoVersioningSystem class was not defining the list_relevant_files method, which meant it as falling back to the VersioningSystemInterface definition which returns None. This commit make the VersioningSystemInterface an ABC and makes all the relevant methods abstract, then implements the methods on the VersioningSystem implementations.
a8113a2 to
ededb1f
Compare
| ) | ||
| logger.debug( | ||
| f"Found {len(network)} network files to report, ({len(unfiltered_network)} without filtering)" | ||
| f"Found {len(network) if network else None} network files to report, ({len(unfiltered_network) if network else None} without filtering)" |
There was a problem hiding this comment.
| f"Found {len(network) if network else None} network files to report, ({len(unfiltered_network) if network else None} without filtering)" | |
| f"Found {len(network) if network else 0} network files to report, ({len(unfiltered_network) if network else 0} without filtering)" |
when the versioning system is
None,list_relevant_filesreturnsNoneso this changes the code in the upload_collector and the network_finder to handle that caseFixes: #641