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

Use IdTagInfo datatype instead of Dict for id_tag_info occurances #278

Merged

Conversation

chan-vince
Copy link
Contributor

@chan-vince chan-vince commented Dec 5, 2021

The Authorize.conf message in the v1.6 spec defines what fields should be present for an IdTagInfo. The dataclass for this already exists in datatypes.py, as of the recently merged #250 PR.

This PR makes use of it by importing the dataclass, and using it instead of the more losely typed typing.Dict.

Copy link
Collaborator

@OrangeTux OrangeTux left a comment

Choose a reason for hiding this comment

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

Thanks!

@chan-vince
Copy link
Contributor Author

Thanks!

Thanks for taking a look. There's actually quite a few similar occurances in dataclasses where things like typing.List and typing.Dict are used instead of the new specific dataclasses.

I'll have a PR to update those soon..

@dinkopehar
Copy link

dinkopehar commented Mar 10, 2022

What happend to this ? I have it like this:

call_result.AuthorizePayload(
            id_tag_info={}
        )

but instead should be something like:

call_result.AuthorizePayload(
            id_tag_info=IdTagInfo(...)
        )

Should the type be changed to IdTagInfo ?

@sonarcloud
Copy link

sonarcloud bot commented Mar 11, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

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
4.8% 4.8% Duplication

@diorcety
Copy link

diorcety commented Nov 2, 2023

Seems pretty correct PR, what is blocking it?

@OrangeTux
Copy link
Collaborator

Nothing blocks it from being merged. Thanks for the notification @diorcety

@OrangeTux OrangeTux merged commit 09ab8c6 into mobilityhouse:master Nov 3, 2023
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants