Update special dataclass handling

[lihu-zhong]

The existing unit tests appear to catch this regression when running with a python version that exhibits this change, so I didn't add an additional protection.

Please let me know I'm missing anything in this patch. Thanks for a great tool!

[lihu-zhong]

The CI failures don't appear to be a regression of this patch. I get these same warnings when running tox against master:

# warning: 

WARNING: Cannot treat a function defined as a local function: "dummy_module.Class.locally_defined_callable_field"  (use @functools.wraps)

WARNING: Cannot resolve forward reference in type annotations of "dummy_module.function_with_unresolvable_annotation": name 'a' is not defined

[adamtheturtle]

The related issue is https://github.com/agronholm/sphinx-autodoc-typehints/issues/123.

Let's also be aware that @agronholm is looking for a new maintainer (https://github.com/agronholm/sphinx-autodoc-typehints/issues/146).

I would very much like to see this issue fixed.
I'm going to have a go at a review perhaps as an attempt to help out but I do not know this codebase well.

[adamtheturtle]

The existing unit tests appear to catch this regression when running with a python version that exhibits this change, so I didn't add an additional protection.

Is it possible to get CI to exhibit those errors? And then have this change fix tests which would otherwise fail in CI?

https://github.com/agronholm/sphinx-autodoc-typehints/pull/155 includes a fix for the CI issues you were hitting.

As a reader, I would expect _is_dataclass to behave basically the same as dataclasses.is_dataclass(obj). However, it does not and I'm not clear why. Perhaps a comment or renaming would help to answer "What is this function meant to do that is different from dataclasses.is_dataclass(obj)?".

[lihu-zhong]

yes. if you run CI on master with a version of python that has these changes (I'm using 3.8.5), it will show WARNING: Cannot treat a function defined as a local function on the dataclass test. Under the same conditions, adding this patch removes that failure.

The difference is the input type. obj in this context is not an instantiated dataclass object. The type is not known until runtime, hence the what variable. I can change the function name to something like _is_generated_dataclass_constructor() if you'd like something more specific?

Thanks for the review!

[adamtheturtle]

My preference (and again, I am not a maintainer or gatekeeper on this project) is to modify test_sphinx_output as so:

• Right now there is:

    warnings = warning.getvalue().strip()
    assert 'Cannot resolve forward reference in type annotations of ' in warnings, warnings

• Instead, there could be something like:

warnings = warning.getvalue().strip().splitlines()
# ...  now assert something about each expected warning

This should fail (not just warn!) on CI and give a heads up ASAP when things break in future.

I can change the function name to something like _is_generated_dataclass_constructor() if you'd like something more specific?

A comment detailing exactly what you just said would be great (for me personally as a reader).

[agronholm]

@lbenezriravin would you mind rebasing against current master? The CI failures should be gone.

[lihu-zhong]

I elaborated in the comment to clarify as per @adamtheturtle 's request. While I agree that the proposed update to the tests is probably better, IMO it's out of scope for this specific patch to refactor the unit tests.

[adamtheturtle]

IMO it's out of scope for this specific patch to refactor the unit tests.

The reason I suggested it is I think that if there is a reported bug and there is no failing test for that bug, then an appropriate scope includes adding (or modifying) a test which would demonstrate that bug, and then fixing that test.

[adamtheturtle]

That said - I would be more than happy to see this merged so I can remove my workaround hacks!

[lihu-zhong]

Sorry for the delay. I see that the unit tests no longer fail when I remove this patch, probably due to the CI fix. You're right, adding a protection is absolutely in scope in that case. I'm a little busy for the next few days, but I'll try to get an update in soon.

[adamtheturtle]

@lbenezriravin If you'd like to share this branch with me I'm happy to add some tests 😄

[lihu-zhong]

Sorry for the delay. I added a protection for this bug and verified that my patch fixes it.

[agronholm]

One or more tests use f-strings which are not compatible with Python 3.5. As 3.5 is now EOL, I will drop it soon, but until I do that please use 3.5 compatible syntax.

[lihu-zhong]

ah, my bad. I don't have 3.5 installed, so those tests were being skipped when I tested locally.

[lihu-zhong]

I'm not really sure how to resolve this -- python 3.5 doesn't support type hinting variables (pep526), which I need in order to catch this bug on later versions of python. Since the tests are a monolith, I'm not seeing a super clean way to skip testing dataclasses on python3.5 (since it doesn't matter there anyways).

[agronholm]

Add this to conftest.py:

import re
import sys

version_re = re.compile(r'_py(\d)(\d)\.py$')


def pytest_ignore_collect(path, config):
    match = version_re.search(path.basename)
    if match:
        version = tuple(int(x) for x in match.groups())
        if sys.version_info < version:
            return True

Then name the test module with a _py36.py suffix.

[lihu-zhong]

Since there's only one test module, wouldn't that skip all tests for python 3.5? How is that different from just removing py35 from tox?

[agronholm]

The point is that you put all the tests requiring 3.6+ syntax in a new test module with the _py36 suffix.

[lihu-zhong]

Alright, I think I broke out the dataclass test as cleanly as possible. Thank you for your help!

edit: removed a rogue .swp file

[agronholm]

Thanks!

[adamtheturtle]

Thank you @agronholm and @lbenezriravin for your hard work on this.

@agronholm - Is it possible to have a release with this change soon? Is there anything that I can do to help?

[agronholm]

I'll try to get a release out within the next couple days.

[adamtheturtle]

Thank you @agronholm 🙏

[agronholm]

v1.11.1 released.