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

Add ImportAnnotations configuration option #653

Merged
merged 10 commits into from
Feb 6, 2022
Merged

Add ImportAnnotations configuration option #653

merged 10 commits into from
Feb 6, 2022

Conversation

teobouvard
Copy link
Contributor

📒 Description

This PR adds a new element ImportAnnotations to the Output section of the configuration file. The goal of this configuration option is to allow schemas introducing collisions between a class member name and its type in the generated bindings to be imported without errors.

Resolves #646

🔗 What I've Done

As discussed in the second part of #646, name collisions originally raised a TypeError because type annotations could not be resolved. However, PEP 563 discusses a way to postpone the evaluation of type hints in such a way that name collisions are acceptable. This behavior is optional in Python 3.7.0b1, and will be mandatory in Python 3.10 (see Future statement definitions).
This configuration option is disabled by default, and enabling it will add the following import at the top of all generated modules.

from __future__ import annotations

This is done by simply prepending the import statement to the default_imports filter used in the module Jinja template.

💬 Comments

Thank you for this library and the effort you're putting towards code quality. Let me know if more tests should be added for this feature.

🛫 Checklist

This allows schemas having collisions between class members name and
type to generate correct Python bindings, by enabling Postponed
Evaluation of Annotations (see PEP 563).
This is done by adding __future__.annotations as a default import to
all generated modules when this option is enabled.
This test generates bindings having a collision between a class member
name and its type, but uses the ImportAnnotations configuration option
to postpone the evaluation of the type hint.
The test then verifies that the binding can be loaded without errors,
and that its colliding member can be accessed.
This helps users diagnose and fix name/type collisions by using the
ImportAnnotations configuration option.
@codecov
Copy link

codecov bot commented Feb 3, 2022

Codecov Report

Merging #653 (6abed7b) into master (16b12b6) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #653   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           94        94           
  Lines         8214      8224   +10     
  Branches      1583      1585    +2     
=========================================
+ Hits          8214      8224   +10     
Impacted Files Coverage Δ
xsdata/formats/dataclass/filters.py 100.00% <100.00%> (ø)
xsdata/models/config.py 100.00% <100.00%> (ø)

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 16b12b6...6abed7b. Read the comment docs.

@teobouvard
Copy link
Contributor Author

So annotations is not part of __future__ in Python 3.6. We could add an include guard checking the current Python version but this feature would be broken on this version, which is not ideal. What do you think ?

Copy link
Owner

@tefra tefra left a comment

Choose a reason for hiding this comment

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

Awesome job, @teobouvard

Can we please change the name of the new option and avoid adding a new project settings file .xsdata.xml for the tests and maybe update the docstring because it will appear in the cli,

docs/faq/why-i-get-a-typeerror-requires-a-single-type.rst Outdated Show resolved Hide resolved
tests/fixtures/annotations/xsdata.xml Outdated Show resolved Hide resolved
schema = filepath.joinpath("model.xsd")
runner = CliRunner()
result = runner.invoke(
cli, [str(schema), f"--config={str(filepath.joinpath('xsdata.xml'))}"]
Copy link
Owner

Choose a reason for hiding this comment

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

All GeneratorOutput options appear in the cli automatically, no need to add a new config file.

❯ xsdata generate --help
...
...
...

Options:
  -r, --recursive                 Search files recursively in the source
                                  directory
  -c, --config TEXT               Project configuration
  -pp, --print                    Print output
  -p, --package TEXT              Target package, default: generated
  -o, --output [dataclasses]      Output format name, default: dataclasses
  --repr / --no-repr              Generate __repr__ method, default: true
  --eq / --no-eq                  Generate __eq__ method, default: true
...
...
...
...
  --import-annotations / --no-import-annotations
                                  Import annotations from :obj:`__future__` in
                                  generated modules, default: false
  --help                          Show this message and exit.

Copy link
Owner

Choose a reason for hiding this comment

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

And fix the help text as well, it comes fromt the docstring

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 it possible to document it in such a way that the HTML docs have the link for the __future__ module, but the CLI help is only text ?

Copy link
Contributor Author

@teobouvard teobouvard Feb 3, 2022

Choose a reason for hiding this comment

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

Maybe there is no need to have implementations details in the docstring. I think the following should be sufficient.

:param postponed_annotations: Enable postponed evaluation of annotations, default: false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All GeneratorOutput options appear in the cli automatically, no need to add a new config file.

That's awesome ! However, I think I still need the configuration file to set the namecase, which should be originalCase for the purpose of the test.

tests/integration/test_annotations.py Show resolved Hide resolved
@tefra
Copy link
Owner

tefra commented Feb 3, 2022

So annotations is not part of __future__ in Python 3.6. We could add an include guard checking the current Python version but this feature would be broken on this version, which is not ideal. What do you think ?

Check the OutputFormat class in the config, maybe we could add something similar there for the GeneratorOutput

def __post_init__(self):
self.validate()
def validate(self):
if self.order and not self.eq:
raise GeneratorConfigError("eq must be true if order is true")
if self.value == "dataclasses" and sys.version_info < (3, 10):
if self.slots:
self.slots = False
warnings.warn(
"slots requires python >= 3.10, reverting...",
CodeGenerationWarning,
)
if self.kw_only:

This informs users that enabling Postponed Evaluation of Annotations is
only available for Python 3.7+.
This commit renames references to the newly introduced feature to
describe _what_ this feature does rather than _how_ it does it.
This allows users to be informed that PostponedAnnotations are only
available in Python 3.7+ by displaying a warning.
@teobouvard teobouvard requested a review from tefra February 3, 2022 18:07
Extracting the Python version into a local variable prevents pyupgrade
from refactoring version checks and changing the test semantics.
@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 4, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@tefra
Copy link
Owner

tefra commented Feb 6, 2022

Awesome job @teobouvard thanks for contributing!

@tefra tefra merged commit eb4b0f1 into tefra:master Feb 6, 2022
@teobouvard teobouvard deleted the feature-annotations branch February 6, 2022 13:50
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.

Imports raise SyntaxError with FieldName as originalCase
2 participants