Copy Blueprints Implementation

[ChihweiLHBird]

Related issue: #2163

[ChihweiLHBird]

@ahopkins The parameters list of the copy method breaks the codeclimate rule: "Function copy has 5 arguments (exceeds 4 allowed)."

Do you think key words parameters will be a better choice? Or can we ignore the codeclimate issue?

[ahopkins]

Ignore that. I need to configure the rules. The defaults might work for an app, but are too restrictive for a framework.

[ChihweiLHBird]

Ignore that. I need to configure the rules. The defaults might work for an app, but are too restrictive for a framework.

Thank you. I think this PR is roughly done, how can I get some feedbacks from you and other members of the community?

[ahopkins]

Sure, I'll take a look. Mark it as ready for review.

@sanic-org/framework thoughts?

[ChihweiLHBird]

Sure, I'll take a look. Mark it as ready for review.

@sanic-org/framework thoughts?

One doubt I got is that in the copy method, should we use the default values of the parameters of the method in the new blueprint if the parameters was not provided by caller? Or we don't change anything in the new blueprint and let it use the old values copied from the old blue print.

Another way to do this, by using key words parameters, we can allow user to change any attribute (methods and variables) in the new blueprint. Something like:

def copy(
        self,
        name: str,
        **kwargs
    ):
        new_bp = deepcopy(self)
        for key, value in kwargs.items():
            if hasattr(new_bp, key):
                new_bp.key = value

Do you guys think it is good idea or not?

[Tronic]

Another way to do this, by using key words parameters, we can allow user to change any attribute (methods and variables) in the new blueprint. Something like:

It is better to explicitly list the parameters. This gives auto completion and error checking in IDEs, not possible if a function takes **kwargs.

[ahopkins]

There are four solutions that I can see:

Pull from kwargs

Pro: Explicit knowledge that the dev intentionally passed a value
Con: No IDE auto-completion

Keyword with None as default value

Pro: Easy to check that a value was intentionally set
Con: Does not allow passing None as a value

Keyword with carrying over existing defaults

Pro: Easy to check that a value is NOT default and set to that
Con: What if you are returning a value back to its default?

Keyword with sentinel as default

Pro: Allows for both None and orig defaults to be passed
Con: Difficult to type annotate

---

Therefore, I suggest a two step approach:

# ./helpers.py
class Default:
    ...


_default = Default()

class Blueprint:
    def copy(
        self,
        name: str,
        *,
        url_prefix: Optional[Union[str, Default]] = _default,
        version: Optional[Union[int, str, float, Default]] = _default,
        strict_slashes: Optional[Union[bool, Default]] = _default,
        version_prefix: Optional[Union[str, Default]] = _default,
    ):
        ...

[ChihweiLHBird]

There are four solutions that I can see:

Pull from kwargs

Pro: Explicit knowledge that the dev intentionally passed a value
Con: No IDE auto-completion

Keyword with None as default value

Pro: Easy to check that a value was intentionally set
Con: Does not allow passing None as a value

Keyword with carrying over existing defaults

Pro: Easy to check that a value is NOT default and set to that
Con: What if you are returning a value back to its default?

Keyword with sentinel as default

Pro: Allows for both None and orig defaults to be passed
Con: Difficult to type annotate

---

Therefore, I suggest a two step approach:

# ./helpers.py
class Default:
    ...


_default = Default()

class Blueprint:
    def copy(
        self,
        name: str,
        *,
        url_prefix: Optional[Union[str, Default]] = _default,
        version: Optional[Union[int, str, float, Default]] = _default,
        strict_slashes: Optional[Union[bool, Default]] = _default,
        version_prefix: Optional[Union[str, Default]] = _default,
    ):
        ...

Hi Adam, thanks for the suggestion. Do you think the default class can be used globally in the framework? We can achieve similar functionality in other functions as well with it, I think.

By the way, I don't quite understand what's the Con of the third solution you mentioned. Can you explain what is the situation of returning a value back to its default? Thank you so much.

[Tronic]

Derive the sentinel type from NoneType, then plain Optional should work for annotation. Seems a bit hackish though, is it a frequent need to set some value to None?

[ChihweiLHBird]

Derive the sentinel type from NoneType, then plain Optional should work for annotation. Seems a bit hackish though, is it a frequent need to set some value to None?

I think generally, we are not able to use NoneType as a class.

[Tronic]

I think generally, we are not able to use NoneType as a class.

Today I learned something new. Looks like None and a few other built in values are not proper objects (in contrast to integers and floats which are).

TypeError: type 'NoneType' is not an acceptable base type

[ahopkins]

>>> class D(type(None)):
...  ...
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: type 'NoneType' is not an acceptable base type

Hardcoded protection: https://github.com/python/cpython/blob/main/Objects/typeobject.c#L2207

---

Off the top of my head, I do not have another use case for _default. Typically object() is the standard sentinel, but typing it is hard, broken, or awkward. Hence the Default class.

---

is it a frequent need to set some value to None?

None is a valid value and could be reused to reset to "default". Since this is entirely new functionality, it is hard to comment on the frequency. However, creating a blueprint with strict_slashes=None is not the same as strict_slashes=False.

bp1 = Blueprint("One", strict_slashes=False)
bp2 = bp1.copy("Two", strict_slashes=None)
group = Blueprint.group(bp1, bp2, strict_slashes=True)

In this use case, the routes on bp2 would have strict_slashes=True (as long as the route definition does not override it).

---

By the way, I don't quite understand what's the Con of the third solution you mentioned. Can you explain what is the situation of returning a value back to its default? Thank you so much.

See the above. If we simply looked for strict_slashes is not None (which is the default valie), then bp2 would end up with strict_slashes=False. Here, bp1 changed the value, but we want to go back to the default value for bp2. Option 3 would not allow us to do this. If, however, we check that strict_slashes != _default, then we can set strict_slahes to a new value, or the original default.

[ChihweiLHBird]

I think the natural way to do copy is keeping everything same as the old object unless the user request a change. So I would suggest we to think about a way that allows users to pass or apply these below to the attributes of the new object:

1. None as a value.
2. The default value of an attribute when the object is created via its constructor with a minimal parameter input.

The second can be achieved by the Default class and object @ahopkins mentioned above. But None value still conflicts with the attributes from the old objects, meaning we cannot use None as the default value of the parameters of the copy method, otherwise we don't know whether or not the None is from the user.

I guess we must take a trade off when implementing this feature, 😂

[ahopkins]

I think the natural way to do copy is keeping everything same as the old object unless the user request a change. So I would suggest we to think about a way that allows users to pass or apply these below to the attributes of the new object:

1. None as a value.

2. The default value of an attribute when the object is created via its constructor with a minimal parameter input.

The second can be achieved by the Default class and object @ahopkins mentioned above. But None value still conflicts with the attributes from the old objects, meaning we cannot use None as the default value of the parameters of the copy method, otherwise we don't know whether or not the None is from the user.

I guess we must take a trade off when implementing this feature, 😂

The default values of all copy args should be _default for that reason.

[ChihweiLHBird]

@ahopkins So, _default means the value of the attributes of the old object?

[ahopkins]

@ahopkins So, _default means the value of the attributes of the old object?

In this case it means "do nothing"

if version != _default:
    # set version on new blueprint

[ChihweiLHBird]

Thank you @ahopkins @Tronic

Can you take a look and see how is it now?

[ahopkins]

Thanks for putting this together. I think it will be a nice add. Make sure to add some unit tests.

[ahopkins]

Okay... now that I closed some of the conversations so it is easier to follow, this LGTM. Just a couple more comments and we should be good to wrap this up. Thanks for all the work on this one!! 🚀

[ahopkins]

Failing test

  >           raise FinalizationError("Cannot finalize router more than once.")

On this line:

           bp6 = bp1.copy(
              name="test_bp6",
              version=6,
              with_registration=True,
  >           version_prefix="/version",
          )
  
  tests/test_blueprint_copy.py:64: 

---

Easy solve:

before the b6 copy, add:

app.router.reset()