Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .coveragerc
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
[run]
omit = *tests*
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,5 @@ docs/_build

# IDEs
/.idea
*.ropeproject
*.swp
1 change: 1 addition & 0 deletions AUTHORS.rst
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@ Contributors
~~~~~~~~~~~~

* Khaled Porlin - https://github.com/porlin72
* Agam Dua - https://github.com/agamdua
50 changes: 50 additions & 0 deletions drf_braces/tests/test_utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
from __future__ import print_function, unicode_literals

import unittest

from drf_braces.utils import (
get_class_name_with_new_suffix,
get_attr_from_base_classes,
)
from rest_framework import fields


class TestUtils(unittest.TestCase):
def test_get_class_name_with_new_suffix(self):
new_name = get_class_name_with_new_suffix(
klass=fields.IntegerField,
existing_suffix='Field',
new_suffix='StrawberryFields'
)
self.assertEqual(new_name, 'IntegerStrawberryFields')

new_name = get_class_name_with_new_suffix(
klass=fields.IntegerField,
existing_suffix='straws',
new_suffix='Blueberries'
)
self.assertEqual(new_name, 'IntegerFieldBlueberries')

def test_get_attr_from_base_classes(self):
Parent = type(str('Parent'), (), {'fields': 'pancakes'})

self.assertEqual(
get_attr_from_base_classes((Parent,), [], 'fields'), 'pancakes'
)

self.assertEqual(
get_attr_from_base_classes(
(Parent,), {'fields': 'mushrooms'}, 'fields'
),
'mushrooms'
)

self.assertEqual(
get_attr_from_base_classes((Parent,), [], '', default='maple_syrup'),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is default used somewhere? I saw that default is provided in the code as FormSerializerOptions. This seems odd to me since we are returning a string or a class.

Refer especially to the docstring

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this does not return str or default. this returns value found in either using these priorities:

  1. dict of attributes (passed in __new__)
  2. in parent bases

this is really meant to mimick:

class A(object):
    foo = 1
class B(B):
    foo = 2

assert type('C', (B,) {'foo': 3}).foo == 3
assert type('C', (B,) {}).foo == 2
assert type('C', (A,) {}).foo == 1

so in https://github.com/agamdua/django-rest-framework-braces/blob/test/utils/drf_braces/serializers/form_serializer.py#L120-L122 I lookup if _options_class is defined as explicit attribute in the to-be created class. if yes, I use that. if not, I try to find it in any of the bases. and if not found there, I return default which uses FormSerializerOptions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So its always intended to be a class?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not following.

get_attr_from_base_classes pretty does what Python does internally when class is already created:

when looking for attribute foo

# stupid implemention which only works for classes
def getattr(obj, attr, default=None):
    for i in attr.mro():  # mro includes itself
        if attr in i.__dict__:
            return i.__dict__['attr']
    if default is not None:
        return default
    raise AttributeError

except in my case I dont have access to mro since class has not been created yet....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it.

'maple_syrup'
)

with self.assertRaises(AttributeError):
get_attr_from_base_classes(
(Parent,), {'fields': 'mushrooms'}, 'catchmeifyoucan'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol catchmeifyoucan. haha

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💃

)
41 changes: 38 additions & 3 deletions drf_braces/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,21 @@ def initialize_class_using_reference_object(reference_object, klass, **kwargs):


def get_class_name_with_new_suffix(klass, existing_suffix, new_suffix):
"""
Generates new name by replacing the existing suffix with a new one.

Args:
klass (type): original class from which new name is generated
existing_suffix (str): the suffix which needs to remain where it is
new_suffix (str): the new suffix desired

Example:
>>> get_class_name_with_new_suffix(FooForm, 'Form', 'NewForm')
'FooNewForm'

Returns:
new_name (str): the name with the new suffix
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

example might be nice:

Example:
    ::

        get_class_name_with_new_suffix(FooForm, 'Form', 'NewForm') == 'FooNewForm'

class_name = klass.__name__

if existing_suffix in class_name:
Expand All @@ -129,7 +144,27 @@ def get_class_name_with_new_suffix(klass, existing_suffix, new_suffix):
return new_name


def get_attr_from_base_classes(bases, attrs, attr, **kwargs):
def get_attr_from_base_classes(bases, attrs, attr, default=None):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is acceptable, I think we can merge.

The rest of the comments were about docstrings

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine. still not a fan but ok.

"""
The attribute is retrieved from the base classes if they are not already
present on the object.

Args:
bases (tuple, list): The base classes for a class.
attrs (dict): The attributes of the class.
attr (str): Specific attribute being looked for.
default (any): Whatever default value is expected if the
attr is not found.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can add explicit default arg. I moved it to **kwargs so that I can detect when its provided and when its not since default=None will mask when people actually want the default to be None...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since default=None will mask when people actually want the default to be None

How will the behavior differ? I would prefer to add it as a kwarg.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

>>> def default(default=None):
...     if default is not None:
...         return default
...     raise ValueError
...
>>> default(default=5)
5
>>> default(default='')
''
>>> default()
Traceback (most recent call last):
  File "<ipython-input-4-7b40379d4ea0>", line 1, in <module>
    default()
  File "<ipython-input-1-234c4a43a49c>", line 4, in default
    raise ValueError
ValueError

>>> default(default=None)
Traceback (most recent call last):
  File "<ipython-input-5-d1b4f539633c>", line 1, in <module>
    default(None)
  File "<ipython-input-1-234c4a43a49c>", line 4, in default
    raise ValueError
ValueError

>>> def default(**kwargs):
...     if 'default' in kwargs:
...         return kwargs['default']
...     raise ValueError
...
>>> default(default=5)
5
>>> default(default='')
''
>>> default(default=None)          # <===== notice this does not blow up now

>>> default()
Traceback (most recent call last):
  File "<ipython-input-12-7b40379d4ea0>", line 1, in <module>
    default()
  File "<ipython-input-8-30e55c8618b5>", line 4, in default
    raise ValueError
ValueError

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like its a bit crazy 😮

On a more practical note though, is this something that can ever affect this repository?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably not but it makes me happy 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woohoo!

happy and we know it


However, I would like to request addition as an explicit kwarg to be accepted. I can push it anytime :)

hmmph

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you can achieve the same with explicit kwarg as illustrated above, sure...

Returns:
attribute value as found in base classes or a default when attribute
is not found and default is provided.

Raises:
AttributeError: When the attribute is not present anywhere in the
call chain hierarchy specified through bases and the attributes
of the class itself
"""
if attr in attrs:
return attrs[attr]

Expand All @@ -139,8 +174,8 @@ def get_attr_from_base_classes(bases, attrs, attr, **kwargs):
except AttributeError:
continue

if 'default' in kwargs:
return kwargs['default']
if default:
return default

raise AttributeError(
'None of the bases have {} attribute'
Expand Down