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

Include documented aliases in output #313

Open
mthuurne opened this issue Dec 4, 2020 · 4 comments · May be fixed by #775
Open

Include documented aliases in output #313

mthuurne opened this issue Dec 4, 2020 · 4 comments · May be fixed by #775
Labels
astbuilder A substantive change is required in the astbuilder flow in order to fix this issue
Milestone

Comments

@mthuurne
Copy link
Contributor

mthuurne commented Dec 4, 2020

From src/twisted/words/protocols/jabber/client.py: (irrelevant parts cut out)

class BasicAuthenticator(xmlstream.ConnectAuthenticator):
    """
    Authenticates an XmlStream against a Jabber server as a Client.

    @cvar INVALID_USER_EVENT: See L{IQAuthInitializer.INVALID_USER_EVENT}.
    @type INVALID_USER_EVENT: L{str}

    @cvar AUTH_FAILED_EVENT: See L{IQAuthInitializer.AUTH_FAILED_EVENT}.
    @type AUTH_FAILED_EVENT: L{str}

    @cvar REGISTER_FAILED_EVENT: Token to signal that registration failed.
    @type REGISTER_FAILED_EVENT: L{str}
    """

    INVALID_USER_EVENT = IQAuthInitializer.INVALID_USER_EVENT
    AUTH_FAILED_EVENT = IQAuthInitializer.AUTH_FAILED_EVENT
    REGISTER_FAILED_EVENT = "//event/client/basicauth/registerfailed"

Currently pydoctor will handle INVALID_USER_EVENT and AUTH_FAILED_EVENT as aliases, which means that it keeps track of the indirections, but doesn't include them in the output. However, as they are documented names, they should be given documentation entries.

From src/twisted/python/constants.py:

# Import and re-export Constantly
from constantly import NamedConstant, ValueConstant, FlagConstant, Names, Values, Flags

__all__ = ["NamedConstant", "ValueConstant", "FlagConstant", "Names", "Values", "Flags"]

Since these names are explicitly exported via __all__, they should end up in the output. But currently the documentation for twisted.python.constants is empty.

So I think aliases should sometimes be Documentables.

@tristanlatr
Copy link
Contributor

I'de like to work on this issue. This is a major thing that pydoctor does not do yet.

I'm just not sure about skipping undocumented aliases tho ?
I think alias should be always given a documentation entry, even when not documented.

@tristanlatr
Copy link
Contributor

tristanlatr commented Jun 25, 2021

To clarify the difference between aliases and constants, we could do the following:

  • create the new alias kind
  • create a is_alias(expr) -> bool function and change ModuleVistor._handleAliasing() method such that, instead of hacking the redirection with ctx._localNameToFullName_map[target] = full_name, it creates a legit Attribute documentable and it returns None, like other _handle* methods.
  • Move the _handleAliasing() calls at the same places we check for the constants, this will allow to spot alias inside a class context, too.
  • first check if the assignment is an alias, then check if it's a constant. This way, the code in your example @mthuurne will be considered aliases and not constants.
    - An alias can could be defined as follow: single assignment that points to a ast.Name or ast.Attribute nodes OR an import and re-export of names that are not part of the current system. I propose to remove the check if target in ctx.contents from _handleAliasing()because this would allow to document things such as the constantly re-export and more generally things that are not from the current scope but we've imported and assigned a alias.
    - The expandName() method should be changed such that it follows the alias when possible.
    - The _importNames() method should be changed such that it creates an alias when a re-exported name is not part of the current system, instead of ignoring it.
  • The alias could have an autogenerated text showing "Alias to <object name that we tried to link/colorize with the PyvalColorizer>" (see Constant values and decorators colorizing #402)

@tristanlatr
Copy link
Contributor

I'm working on this issue and I have a somewhat generic question:

import sys
Class A:
    if sys.version_info[0] < 3:
        alias = B.a
    else:
        alias = B.b
Class B:
    a = 3
    b = 4
try:
    import .ssl as _ssl
except ImportError:
    alias = None
else:
    alias = _ssl

Should the alias attribute be documented? Or should we ignore (still keeping track of the indirection) it in both cases because the assignment is not on the root level of it's context ?

Meaning that, changing the declaration as follows would mean that it would be documented.

import sys
Class A:
    alias = None
    if sys.version_info[0] < 3:
        alias = B.a
    else:
        alias = B.b
Class B:
    a = 3
    b = 4
alias = None
try:
    import .ssl as _ssl
except ImportError:
    alias = None
else:
    alias = _ssl

Thanks for answers!

PS : I'm asking because there are lots of these cases (especially for modules - but I assume it should work the same for classes) in Twisted and it looks like it's mostly used for sorting out imports that might raise an ImportError. Aliases were originally not created a Documentable entry in the _handleAliasing function, so this was not creating a lot of noise in the documentation , now that we do, it's creating lots of entries that are not really worth it documenting. But at the same time they might be example where we would like to document stuff inside an if block...

@tristanlatr
Copy link
Contributor

tristanlatr commented Jun 27, 2021

Now that I'm thinking about it, I think we should probably create an alias entry even if the aliases are not defined at the root level of the context.

But only show them if they are documented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astbuilder A substantive change is required in the astbuilder flow in order to fix this issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants