Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Second layer defined functions seem not to be checked #293

Closed
conrad-stork-basf opened this issue Apr 8, 2024 · 4 comments · Fixed by #298
Closed

Second layer defined functions seem not to be checked #293

conrad-stork-basf opened this issue Apr 8, 2024 · 4 comments · Fixed by #298
Assignees
Labels
bug Something isn't working enhancement New feature or request

Comments

@conrad-stork-basf
Copy link

Hi @jshwi,

we are using docsig for quite a while now to check our code documentation and we are very happy with the tool. However, we have observed that it seems not to check second layer defined functions. In same cases one could think about such scenarios where functions are defined only under certain conditions. Examples of code that could be imagined:

def my_function(argument: int = 42) -> int:
    """
    Function that prints a message and returns the argument + 1

    Parameters
    ----------
    argument : int, optional
        The input argument, by default 42

    Returns
    -------
    int
        The input argument + 1
    """
    print("Hello from a function")
    print(argument)
    return argument + 1


if True:
    my_function(42)
    def my_external_function(argument: int = 42) -> int:
        print("Hello from an external function")
        print(argument)
        return argument + 42

or

def my_function(argument: int = 42) -> int:
    """
    Function that prints a message and returns the argument + 1

    Parameters
    ----------
    argument : int, optional
        The input argument, by default 42

    Returns
    -------
    int
        The input argument + 1
    """
    print("Hello from a function")
    print(argument)
    def my_external_function(argument: int = 42) -> int:
        print("Hello from an external function")
        print(argument)
        return argument + 42
    return argument + 1```

I would like to ask if this is expected behavior and if so if there are any plans to also check other layers than the first one? For us this would be really helpful 😃

Thanks in advance and Best
Conrad

@jshwi
Copy link
Owner

jshwi commented Apr 12, 2024

Hi @conrad-stork-basf

Thank you for taking the time to open this issue, I'm happy to hear you have found a use for this package, and I appreciate your interest in improving it

Docsig is pretty specific in what it does, but there's a lot of room for customisation. I'll review this issue, and the code you provided a little more, and I'll get back to you as soon as I can (once I have some additional info I can provide)

Cheers!
Stephen

@jshwi jshwi self-assigned this Apr 12, 2024
jshwi added a commit that referenced this issue Apr 14, 2024
Functions and classes inside an if block will now be checked

Signed-off-by: Stephen Whitlock <stephen@jshwisolutions.com>
jshwi added a commit that referenced this issue Apr 14, 2024
Signed-off-by: Stephen Whitlock <stephen@jshwisolutions.com>
@jshwi jshwi linked a pull request Apr 14, 2024 that will close this issue
jshwi added a commit that referenced this issue Apr 14, 2024
Signed-off-by: Stephen Whitlock <stephen@jshwisolutions.com>
@jshwi jshwi added bug Something isn't working enhancement New feature or request labels Apr 14, 2024
jshwi added a commit that referenced this issue Apr 14, 2024
Signed-off-by: Stephen Whitlock <stephen@jshwisolutions.com>
@jshwi
Copy link
Owner

jshwi commented Apr 14, 2024

Hi @conrad-stork-basf

For example one, I've decided that it goes against the expected behaviour. If the user was to want to skip an indented function they can prefix it with an underscore. That will now be a valid check by default after this merge, and has been Fixed

For example two, it is expected that the function is skipped, as it is not in the global scope and exists within the function as sort of a private function or class. I understand that yourself and your team (and other users) may want to document and check these too, so I've Added a new flag, -N/--check-nested. Please pass this flag after you've updated docsig to the latest release (or add it to your pyproject.toml to run this check by default)

I'll include these changes with v0.47.0 which should be available on PyPI when you see this.

Any issues please don't hesitate to open another issue

Thanks again!
Stephen

@conrad-stork-basf
Copy link
Author

Hi @conrad-stork-basf

For example one, I've decided that it goes against the expected behaviour. If the user was to want to skip an indented function they can prefix it with an underscore. That will now be a valid check by default after this merge, and has been Fixed

For example two, it is expected that the function is skipped, as it is not in the global scope and exists within the function as sort of a private function or class. I understand that yourself and your team (and other users) may want to document and check these too, so I've Added a new flag, -N/--check-nested. Please pass this flag after you've updated docsig to the latest release (or add it to your pyproject.toml to run this check by default)

I'll include these changes with v0.47.0 which should be available on PyPI when you see this.

Any issues please don't hesitate to open another issue

Thanks again! Stephen

Hi Stephen,

thanks a lot for the implementation. I have already tested it and it works like a charm. Also thanks for the additional flag, I think this completely makes sense.

If we have any other issues we will let you know, but at the moment everything is working very smooth!

Thanks again and Best
Conrad

@jshwi
Copy link
Owner

jshwi commented Apr 15, 2024

Great! Thanks for confirming @conrad-stork-basf 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants