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

Generated facade code types are too wide #1249

Open
dimaqq opened this issue Jan 6, 2025 · 2 comments
Open

Generated facade code types are too wide #1249

dimaqq opened this issue Jan 6, 2025 · 2 comments
Labels
kind/bug indicates a bug in the project

Comments

@dimaqq
Copy link
Contributor

dimaqq commented Jan 6, 2025

Description

According to
https://github.com/dimaqq/juju-schema-analysis/blob/0acd07573bb2c192785796381f0594470ef296d8/schemas-juju-3.6.0.model-user.txt#L741C1-L753C12

Application.Get().charm is an str

However, the generated facade code accepts None|str|bytes:

if charm_ is not None and not isinstance(charm_, (bytes, str)):
raise Exception(f"Expected charm_ to be a str, received: {type(charm_)}")

The type checker rightfully infers None|str|bytes for the .charm attribute.

Urgency

Casually reporting

Python-libjuju version

3.6.1.0

Juju version

any

Reproduce / Test

charm_name: str = (await Application.Get()).charm
@dimaqq dimaqq added the kind/bug indicates a bug in the project label Jan 6, 2025
@dimaqq
Copy link
Contributor Author

dimaqq commented Jan 6, 2025

P.S. I imagine that the codegen path is generic and may be used for other things that are:

  • nullable (because, like, everything is in Juju or something?)
  • not representable as a string (?)

Thus it may not be a trivial change.

@dimaqq
Copy link
Contributor Author

dimaqq commented Jan 6, 2025

Likewise, things are expected to be arrays of objects are also allowed to be:

  • None (because everything is nullable?)
  • str (eh? why?)
  • bytes (because any str may also be bytes or what?)

For example:

class ApplicationMetricCredentials(Type):
_toSchema = {"creds": "creds"}
_toPy = {"creds": "creds"}
def __init__(self, creds=None, **unknown_fields):
"""Creds : typing.Sequence[~ApplicationMetricCredential]"""
creds_ = [ApplicationMetricCredential.from_json(o) for o in creds or []]
# Validate arguments against known Juju API types.
if creds_ is not None and not isinstance(creds_, (bytes, str, list)):
raise Exception(
f"Expected creds_ to be a Sequence, received: {type(creds_)}"
)

Schema:

https://github.com/dimaqq/juju-schema-analysis/blob/0acd07573bb2c192785796381f0594470ef296d8/schemas-juju-3.6.0.model-user.txt#L968-L973

P.S. would this actually be valid for arguments (and not results), is it OK to pass formatted JSON as str or bytes?

The same code appears to generate this for results as well:

Schema: https://github.com/dimaqq/juju-schema-analysis/blob/0acd07573bb2c192785796381f0594470ef296d8/schemas-juju-3.6.0.model-user.txt#L1094-L1098

Code:

class BlockResults(Type):
_toSchema = {"results": "results"}
_toPy = {"results": "results"}
def __init__(self, results=None, **unknown_fields):
"""Results : typing.Sequence[~BlockResult]"""
results_ = [BlockResult.from_json(o) for o in results or []]
# Validate arguments against known Juju API types.
if results_ is not None and not isinstance(results_, (bytes, str, list)):

Here, the extra types don't make sense.
It could be the artefact of same codegen being used for arguments and results, maybe?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug indicates a bug in the project
Projects
None yet
Development

No branches or pull requests

1 participant