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

Allow schema_extra to be a classmethod #1306

Merged
merged 10 commits into from
Apr 15, 2020

Conversation

andriilahuta
Copy link
Contributor

Change Summary

Allow schema_extra to be a classmethod.

Related issue number

Resolves #1305

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • changes/<pull request or issue id>-<github username>.md file added describing change
    (see changes/README.md for details)

@andriilahuta
Copy link
Contributor Author

I find it far easier to remove the check altogether, than tinker with method inspection on class object. Not sure what is the best way to make mypy happy though.
@samuelcolvin wdyt?

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

please can you fix mypy. Also what happens if schema_extra is a instance method? Does it work or break? Let's add a test.

@@ -1534,20 +1534,6 @@ def schema_extra(schema):
assert Model.schema() == {'title': 'Model', 'type': 'override'}


def test_model_with_schema_extra_callable_classmethod_asserts():
Copy link
Member

Choose a reason for hiding this comment

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

let's keep this test but demonstrate that it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@codecov
Copy link

codecov bot commented Mar 18, 2020

Codecov Report

Merging #1306 into master will increase coverage by 0.10%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master     #1306      +/-   ##
===========================================
+ Coverage   99.89%   100.00%   +0.10%     
===========================================
  Files          21        21              
  Lines        3708      3732      +24     
  Branches      731       737       +6     
===========================================
+ Hits         3704      3732      +28     
+ Misses          2         0       -2     
+ Partials        2         0       -2     
Impacted Files Coverage Δ
pydantic/schema.py 100.00% <ø> (+0.55%) ⬆️
pydantic/errors.py 100.00% <0.00%> (ø)
pydantic/fields.py 100.00% <0.00%> (ø)
pydantic/typing.py 100.00% <0.00%> (ø)
pydantic/decorator.py 100.00% <0.00%> (ø)
pydantic/validators.py 100.00% <0.00%> (ø)
pydantic/utils.py 100.00% <0.00%> (+1.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 938335c...9210fca. Read the comment docs.

@andriilahuta
Copy link
Contributor Author

So, I've added a test for instance methods, but I'm not sure how useful it is.

Lint is failing due to obsolete pyflakes version (fixed in master, but not yet released on pypi). How should we proceed?

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

nearly there.

pydantic/main.py Outdated
pass

@overload
def __call__(self, schema: Dict[str, Any], model: Type['Model']) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Surely you don't need both this case and, the one below?

Better to use model_class than model to be clear.

You need to add . # noqa F811 to get linting to pass, see other examples in the code base

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -89,7 +104,7 @@ class BaseConfig:
getter_dict: Type[GetterDict] = GetterDict
alias_generator: Optional[Callable[[str], str]] = None
keep_untouched: Tuple[type, ...] = ()
schema_extra: Union[Dict[str, Any], Callable[[Dict[str, Any]], None]] = {}
schema_extra: Union[Dict[str, Any], 'SchemaExtraCallable'] = {}
Copy link
Member

Choose a reason for hiding this comment

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

maybe you could test this works by adding a to tests/mypy/modules/success.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could use some help here. I've been trying something like this to pass

class ConfigSchemaExtraCallable(BaseConfig['MyModel']):
    @staticmethod
    def schema_extra(schema: Dict[str, Any], model_class: Type['MyModel']) -> None:
        pass

to no avail.
Even when I made BaseConfig generic, mypy still complained about signature incompatibility.

@samuelcolvin
Copy link
Member

spent ages trying to improve on this, but nothing seems to work.

Let's go with this.

@samuelcolvin samuelcolvin merged commit 7bebf2b into pydantic:master Apr 15, 2020
@andriilahuta andriilahuta deleted the schema_extra_classmethod branch April 18, 2020 01:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

schema_extra should be allowed to be a classmethod
2 participants