attempt for doing camel case check in all classes #1385
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1385 +/- ##
==========================================
- Coverage 90.59% 90.48% -0.12%
==========================================
Files 117 117
Lines 14265 14265
==========================================
- Hits 12924 12907 -17
- Misses 1341 1358 +17 |
tests/tests_unit/test_meta.py
Outdated
| @@ -1,5 +1,7 @@ | |||
| from pathlib import Path | |||
|
|
|||
| ALL_FILEPATHS = Path("cognite/client/").glob("**/*.py") | |||
There was a problem hiding this comment.
I just discovered there's a rglob version
There was a problem hiding this comment.
Ooo! Like in this link? There is a comment in there about how apparently os.walk is faster (although this person also had ~10K files to go through. Makes me wonder what the differences are under the hood and when the payoff is worth it. But yes, will update to rglob. I can search for other globs that can be replaced rglob if you like? I will replaced any patterns like .glob("**/*.py") with .rglob("*.py"), right?
tests/tests_unit/test_meta.py
Outdated
| if "tests/" in str(filepath): | ||
| with filepath.open("r") as file: | ||
| for line_num, line in enumerate(file.readlines()): | ||
| if ".dump(camel_case" in line: |
There was a problem hiding this comment.
This would skip files with arguments splatted to multiple lines. I would attack this from either AST parsing, or using the inspect module on the class objects, something along these lines:
from tests.utils import all_subclasses
for cls in all_subclasses(CogniteResource):
if method := getattr(cls, "to_pandas"):
if param := inspect.signature(method).parameters.get("camel_case"):
assert param.default is False- add more base classes that we want to be easily dataframe-convertible.
There was a problem hiding this comment.
I guess we don't need getattr when we know all subclasses have this method... lol whoopsie. But for a general class, it would be beneficial ofc (with an additional third-argument None to not error when missing).
There was a problem hiding this comment.
Yeah, we talked about some of this offline and on Slack (although good to document here). Just a bit busy with customer stuff but will get to it soon.
… and reintroduced getattr check
…ython into doge11 Merged from master
tests/tests_unit/test_meta.py
Outdated
|
|
||
| @pytest.mark.parametrize("cls", [CogniteResource, CogniteResourceList]) | ||
| def test_ensure_all_tests_use_camel_case_except_dump(cls): | ||
| method = "to_pandas" |
There was a problem hiding this comment.
This seems a bit excessive?
There was a problem hiding this comment.
I make a variable for (almost) anything that is needed more than once. It is used three times. 🤷
tests/tests_unit/test_meta.py
Outdated
| def test_ensure_all_tests_use_camel_case_except_dump(cls): | ||
| method = "to_pandas" | ||
| err_msg = "Class: '{}' for method {} does not default to False." | ||
| for cls in all_subclasses(cls): |
There was a problem hiding this comment.
Perhaps rename the base class so that the variable names are different
There was a problem hiding this comment.
Sorry, yes, my bad.
tests/tests_unit/test_meta.py
Outdated
| if cls_method := getattr(cls, method, False): | ||
| continue |
There was a problem hiding this comment.
You need to check the logic on this one 😉
There was a problem hiding this comment.
Yeah, I think I noticed it before pushing, but then I got distracted and forgot to update. 😅
tests/tests_unit/test_meta.py
Outdated
|
|
||
|
|
||
| @pytest.mark.parametrize("cls", [CogniteResource, CogniteResourceList]) | ||
| def test_ensure_all_tests_use_camel_case_except_dump(cls): |
There was a problem hiding this comment.
| def test_ensure_all_tests_use_camel_case_except_dump(cls): | |
| def test_ensure_all_to_pandas_methods_use_snake_case(cls): |
Description
Dodgy attempt to address this one. Please correct me if I have misunderstood the task. :)
Checklist:
If a new method has been added it should be referenced in cognite.rst in order to generate docs based on its docstring.