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

Group Attributes of Class and Object elements of Google Wallet, to make it more maintainable. #6

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

loechel
Copy link
Contributor

@loechel loechel commented Apr 22, 2024

Group Attributes of Class and Object elements of Google Wallet to make it mor maintainable.

)
if path.exists():
self._credentials_file = path
return self._credentials_file

Copy link
Contributor

Choose a reason for hiding this comment

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

This has to be documented.

@@ -394,9 +400,9 @@ def save_link(
}
signer = crypt.RSASigner.from_service_account_file(session_manager.credentials_file)
jwt_string = jwt.encode(signer, claims).decode("utf-8")
logger.debug(
"JWT-Length: %d, is less than recommenden 1800: %s",
logger.warning(
Copy link
Contributor

@jensens jensens Apr 26, 2024

Choose a reason for hiding this comment

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

I am not sure if this is worth a warning. It works anyway if it is longer.

@jensens
Copy link
Contributor

jensens commented Apr 26, 2024

This needs also:

  • major version bump.
  • change log entry
  • updates in documentation repository.

@@ -170,6 +170,8 @@ def update(
url=session_manager.url(name, f"/{resource_id}"),
data=verified_json.encode("utf-8"),
)
logger.debug(verified_json.encode("utf-8"))
# print(verified_json.encode("utf-8"))
Copy link
Contributor

Choose a reason for hiding this comment

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

This commented print should go before merge.

@@ -12,7 +12,7 @@ class GoogleWalletModel(BaseModel):
"""

model_config = ConfigDict(
# extra="forbid",
extra="forbid",
# extra="ignore",
# use_enum_values=True,
Copy link
Contributor

Choose a reason for hiding this comment

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

@loechel
Copy link
Contributor Author

loechel commented Apr 29, 2024

@simon-lund please make a detailed review and if possible also make additional commits.

This Pull-Request changes the default behavior of eduTAP.wallet_google only to accept model attributes to be set if they are explicitly in the model. This follows PEP 20 Zen of Python: explicit is better than implicit

Each model element should have a reference to its documentation and all relevant attributes.
Please check if all attributes are modeled.

Co-authored-by: Jens W. Klein <jk@kleinundpartner.at>
@jensens
Copy link
Contributor

jensens commented May 2, 2024

Before merging, a PR to update https://github.com/edutap-eu/documentation/tree/main/source/packages/edutap_wallet_google is needed over there.

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.

2 participants