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.

Sign up for GitHub

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

fix: don't use lexical sorting for version numbers in codegen #1168

Closed
wants to merge 1 commit into from
Closed

fix: don't use lexical sorting for version numbers in codegen #1168

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 14 additions & 3 deletions juju/client/facade.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
from collections import defaultdict
from glob import glob
from pathlib import Path
from typing import Any, Mapping, Sequence, TypeVar
from typing import Any, Dict, List, Mapping, Sequence, Tuple, TypeVar

import typing_inspect

Expand Down Expand Up @@ -926,11 +926,22 @@ def generate_definitions(schemas):
return definitions


def generate_facades(schemas):
def sortable_schema_version(version_string: str) -> Tuple[int, int, int]:
"""Return a sorting key in the form (major, minor, micro) from a version string."""
# 'latest' is special cased in load_schemas and should come last
if version_string == 'latest':
return (9000, 9000, 9000)
Comment on lines +931 to +933
Copy link
Contributor

Choose a reason for hiding this comment

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

Why special-case latest?
We don't use files with the "latest" marker, do we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Late reply, but I special cased it in sorting because the code that loads in schemas special cases it as a permitted version. There was no need to special casing in sorting when sorting was simply lexicographic, since "latest" would sort after all the numerical strings.

# raise ValueError if string isn't in the format A.B.C
major, minor, micro = version_string.split('.')
Copy link
Contributor

@dimaqq dimaqq Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

wdyt about using what already do for the client and server version:

from packaging import version
version.parse(server_version)

juju/client/connector.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.

Sounds like a good idea, if you want to take care of it to push this over the finish line that would be cool, otherwise I'll hopefully get to it some time during the sprint

# raise ValueError if major, minor, and micro aren't int strings
return (int(major), int(minor), int(micro))


def generate_facades(schemas: Dict[str, List[Schema]]) -> Dict[str, Dict[int, codegen.Capture]]:
captures = defaultdict(codegen.Capture)

# Build the Facade classes
for juju_version in sorted(schemas.keys()):
for juju_version in sorted(schemas.keys(), key=sortable_schema_version):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the idea here to go from oldest to newest because the latter iteration overwrites the ealier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Email reply isn't as cool as I was hoping. But yeah, I think it does overwrite.
The relevant line is:

 captures[schema.version].clear(cls_name)

With this PR I'm just slavishly trying to keep the original intent working. But maybe this being technically broken is an opportunity for us to make some bigger changes?

Certainly in terms of implementation it would be nice to get rid of all the inheriting from dict, and have those classes just do operations on an internal dict instead -- at most implement the specific dict methods that are expected to be used and forward them to the internal dict rather than inheriting every dict method and overriding some of them ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh and note that schema.version is a facade version, like 7.

juju_version is the Juju schema version, like '3.1.10'.

for schema in schemas[juju_version]:
cls, source = buildFacade(schema)
cls_name = "{}Facade".format(schema.name)
Expand Down
Loading