Skip to main content
deleted 3693 characters in body
Source Link
Peilonrayz
  • 43.5k
  • 7
  • 76
  • 155
  1. To remove the need for a comment change parameters.popitem(0) to parameters.pop('self').
  2. to_positional_argument_string is a mouth full, and people would understand your code easier if you just use repr.
  3. to_keyword_argument_string is another mouth full and so you could call it kw_repr.
  4. You may want to add a comment explaining what isinstance(field, abc.Iterator) is for. This is because at first it confused me.
  5. [Controversial] You can change your code so that you don't need to be passed constructor_or_initializer. To do this you'd create parameters when __repr__ is first used.

Your code has, what I think is, a major unusable bug. The parameter name and the property name have to be the same with this. Given that a lot of the time I make the argument private by using _{name}, your code will not work. Here are a couple of ideas on how to 'fix' this:

  • Wrap __init__ with code that checks against the parameters and the properties. Using something similar to test in this answer of mine you should be able to get all the parameters with ease. After this you could compare against the object by looping through __dict__. (If the object isn't using __slots__)

    This would allow you to create a mapping from the parameter and property name. I'd throw away any 'links' that aren't 100%. And so I'd only do this for parameters that link to just on property, and vice versa.

    This would work for the following __init__ however:

     def __init__(self, a, b, c):
         self._a = a
         self._b = b
         self._c = c
    
  • This however still has problems, as outlined above, and so being able to pass a dictionary with these links could help too.

     __repr__ = generate_repr(__init__, links={'a': '_a', 'b': '_b', 'c': '_c'})
    
  • Still the above could still be wrong if say a is converted from one datatype to another and so you may need to allow passing a 'decoder'.

And so implementing just the bottom two can allow:

class B:
    def __init__(self, a, b, c):
        self.a = a
        self._b = b
        self._c = c + b

    __repr__ = generate_repr(
        __init__,
        links={'b': '_b', 'c': lambda o: o._c - o._b}
    )

print(repr(B(1, 2, 3)))
# B(1, 2, 3)
def generate_repr(constructor_or_initializer: Union[Constructor, Initializer],
                  *,
                  field_seeker: Callable[[Domain, str], Any] = getattr,
                  links: Optional[Dict[str, Union[Callable[[Domain], Any], str]]] = None
                  ) -> Map[Domain, str]:
    signature = inspect.signature(constructor_or_initializer)
    parameters = OrderedDict(signature.parameters)
    parameters.pop('self')
    kw_repr = '{}={!r}'.format
    if links is None:
        links = {}

    def __repr__(self: Domain) -> str:
        return (type(self).__qualname__
                + '(' + ', '.join(to_arguments_strings(self)) + ')')

    def to_arguments_strings(object_: Domain) -> Iterable[str]:
        for parameter_name, parameter in parameters.items():
            name = links.get(parameter_name, parameter_name)
            if isinstance(name, str):
                field = field_seeker(object_, name)
            else:
                field = name(object_)

            if parameter.kind == inspect._VAR_POSITIONAL:
                if isinstance(field, abc.Iterator):
                    yield '...'
                else:
                    yield from map(repr, field)
            elif parameter.kind == inspect._VAR_KEYWORD:
                yield from map(kw_repr, field.keys(), field.values())
            elif parameter.kind in {inspect._POSITIONAL_ONLY,
                                    inspect._POSITIONAL_OR_KEYWORD}:
                yield repr(field)
            else:
                yield kw_repr(parameter_name, field)

    return __repr__
  1. to_positional_argument_string is a mouth full, and people would understand your code easier if you just use repr.
  2. to_keyword_argument_string is another mouth full and so you could call it kw_repr.
  3. You may want to add a comment explaining what isinstance(field, abc.Iterator) is for. This is because at first it confused me.
  4. [Controversial] You can change your code so that you don't need to be passed constructor_or_initializer. To do this you'd create parameters when __repr__ is first used.
  1. To remove the need for a comment change parameters.popitem(0) to parameters.pop('self').
  2. to_positional_argument_string is a mouth full, and people would understand your code easier if you just use repr.
  3. to_keyword_argument_string is another mouth full and so you could call it kw_repr.
  4. You may want to add a comment explaining what isinstance(field, abc.Iterator) is for. This is because at first it confused me.
  5. [Controversial] You can change your code so that you don't need to be passed constructor_or_initializer. To do this you'd create parameters when __repr__ is first used.

Your code has, what I think is, a major unusable bug. The parameter name and the property name have to be the same with this. Given that a lot of the time I make the argument private by using _{name}, your code will not work. Here are a couple of ideas on how to 'fix' this:

  • Wrap __init__ with code that checks against the parameters and the properties. Using something similar to test in this answer of mine you should be able to get all the parameters with ease. After this you could compare against the object by looping through __dict__. (If the object isn't using __slots__)

    This would allow you to create a mapping from the parameter and property name. I'd throw away any 'links' that aren't 100%. And so I'd only do this for parameters that link to just on property, and vice versa.

    This would work for the following __init__ however:

     def __init__(self, a, b, c):
         self._a = a
         self._b = b
         self._c = c
    
  • This however still has problems, as outlined above, and so being able to pass a dictionary with these links could help too.

     __repr__ = generate_repr(__init__, links={'a': '_a', 'b': '_b', 'c': '_c'})
    
  • Still the above could still be wrong if say a is converted from one datatype to another and so you may need to allow passing a 'decoder'.

And so implementing just the bottom two can allow:

class B:
    def __init__(self, a, b, c):
        self.a = a
        self._b = b
        self._c = c + b

    __repr__ = generate_repr(
        __init__,
        links={'b': '_b', 'c': lambda o: o._c - o._b}
    )

print(repr(B(1, 2, 3)))
# B(1, 2, 3)
def generate_repr(constructor_or_initializer: Union[Constructor, Initializer],
                  *,
                  field_seeker: Callable[[Domain, str], Any] = getattr,
                  links: Optional[Dict[str, Union[Callable[[Domain], Any], str]]] = None
                  ) -> Map[Domain, str]:
    signature = inspect.signature(constructor_or_initializer)
    parameters = OrderedDict(signature.parameters)
    parameters.pop('self')
    kw_repr = '{}={!r}'.format
    if links is None:
        links = {}

    def __repr__(self: Domain) -> str:
        return (type(self).__qualname__
                + '(' + ', '.join(to_arguments_strings(self)) + ')')

    def to_arguments_strings(object_: Domain) -> Iterable[str]:
        for parameter_name, parameter in parameters.items():
            name = links.get(parameter_name, parameter_name)
            if isinstance(name, str):
                field = field_seeker(object_, name)
            else:
                field = name(object_)

            if parameter.kind == inspect._VAR_POSITIONAL:
                if isinstance(field, abc.Iterator):
                    yield '...'
                else:
                    yield from map(repr, field)
            elif parameter.kind == inspect._VAR_KEYWORD:
                yield from map(kw_repr, field.keys(), field.values())
            elif parameter.kind in {inspect._POSITIONAL_ONLY,
                                    inspect._POSITIONAL_OR_KEYWORD}:
                yield repr(field)
            else:
                yield kw_repr(parameter_name, field)

    return __repr__
  1. to_positional_argument_string is a mouth full, and people would understand your code easier if you just use repr.
  2. to_keyword_argument_string is another mouth full and so you could call it kw_repr.
  3. You may want to add a comment explaining what isinstance(field, abc.Iterator) is for. This is because at first it confused me.
  4. [Controversial] You can change your code so that you don't need to be passed constructor_or_initializer. To do this you'd create parameters when __repr__ is first used.
Source Link
Peilonrayz
  • 43.5k
  • 7
  • 76
  • 155

Honestly it looks like most of your code can't be simplified. However I found:

  1. To remove the need for a comment change parameters.popitem(0) to parameters.pop('self').
  2. to_positional_argument_string is a mouth full, and people would understand your code easier if you just use repr.
  3. to_keyword_argument_string is another mouth full and so you could call it kw_repr.
  4. You may want to add a comment explaining what isinstance(field, abc.Iterator) is for. This is because at first it confused me.
  5. [Controversial] You can change your code so that you don't need to be passed constructor_or_initializer. To do this you'd create parameters when __repr__ is first used.

Your code has, what I think is, a major unusable bug. The parameter name and the property name have to be the same with this. Given that a lot of the time I make the argument private by using _{name}, your code will not work. Here are a couple of ideas on how to 'fix' this:

  • Wrap __init__ with code that checks against the parameters and the properties. Using something similar to test in this answer of mine you should be able to get all the parameters with ease. After this you could compare against the object by looping through __dict__. (If the object isn't using __slots__)

    This would allow you to create a mapping from the parameter and property name. I'd throw away any 'links' that aren't 100%. And so I'd only do this for parameters that link to just on property, and vice versa.

    This would work for the following __init__ however:

     def __init__(self, a, b, c):
         self._a = a
         self._b = b
         self._c = c
    
  • This however still has problems, as outlined above, and so being able to pass a dictionary with these links could help too.

     __repr__ = generate_repr(__init__, links={'a': '_a', 'b': '_b', 'c': '_c'})
    
  • Still the above could still be wrong if say a is converted from one datatype to another and so you may need to allow passing a 'decoder'.

And so implementing just the bottom two can allow:

class B:
    def __init__(self, a, b, c):
        self.a = a
        self._b = b
        self._c = c + b

    __repr__ = generate_repr(
        __init__,
        links={'b': '_b', 'c': lambda o: o._c - o._b}
    )

print(repr(B(1, 2, 3)))
# B(1, 2, 3)
def generate_repr(constructor_or_initializer: Union[Constructor, Initializer],
                  *,
                  field_seeker: Callable[[Domain, str], Any] = getattr,
                  links: Optional[Dict[str, Union[Callable[[Domain], Any], str]]] = None
                  ) -> Map[Domain, str]:
    signature = inspect.signature(constructor_or_initializer)
    parameters = OrderedDict(signature.parameters)
    parameters.pop('self')
    kw_repr = '{}={!r}'.format
    if links is None:
        links = {}

    def __repr__(self: Domain) -> str:
        return (type(self).__qualname__
                + '(' + ', '.join(to_arguments_strings(self)) + ')')

    def to_arguments_strings(object_: Domain) -> Iterable[str]:
        for parameter_name, parameter in parameters.items():
            name = links.get(parameter_name, parameter_name)
            if isinstance(name, str):
                field = field_seeker(object_, name)
            else:
                field = name(object_)

            if parameter.kind == inspect._VAR_POSITIONAL:
                if isinstance(field, abc.Iterator):
                    yield '...'
                else:
                    yield from map(repr, field)
            elif parameter.kind == inspect._VAR_KEYWORD:
                yield from map(kw_repr, field.keys(), field.values())
            elif parameter.kind in {inspect._POSITIONAL_ONLY,
                                    inspect._POSITIONAL_OR_KEYWORD}:
                yield repr(field)
            else:
                yield kw_repr(parameter_name, field)

    return __repr__