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

(PC-20049)[API] refactor: improve offer ExtraData #5028

Merged
merged 7 commits into from
Jan 26, 2023
Merged

Conversation

viconnex
Copy link
Contributor

@viconnex viconnex commented Jan 23, 2023

Lien vers le ticket Jira : https://passculture.atlassian.net/browse/PC-20049

image

Limitations

Malheureusement on ne peut pas :

  • utiliser une variable comme clé (cf cette issue)
  • typer les propriétés d'un TypedDict avec le type TypedDict (cf cette issue)
  • caster un TypedDict en dict (cf cette issue)
  • Le type de extra_data.get("diffusionVersion", "") est str | None
  • pas de validation du typage quand on appelle extraData en tant que propriété de classe (ex: offers_models.Product.extraData["isbnz"].astext)

@viconnex viconnex force-pushed the pc-20049-extra-data branch 2 times, most recently from 042ece7 to 1e02b30 Compare January 23, 2023 16:40
@github-actions
Copy link
Contributor

Debt collector report:

null

null

null

Previous debt : 0
Current debt : 0

Debt decreased by 0 Points

To get a file by file report, please run debt-collector check --changed-since="[REVISION]"

@viconnex viconnex changed the title (PC-20049)[API] refactor: organize offers models fields (PC-20049)[API] refactor: improve ExtraData Jan 23, 2023
@viconnex viconnex force-pushed the pc-20049-extra-data branch 2 times, most recently from 438178e to 219fb84 Compare January 24, 2023 17:44
@github-actions
Copy link
Contributor

github-actions bot commented Jan 24, 2023

mypy cop report: 943 (master) ↘ 940 (your branch)

Have yourself a merry little break and learn a few facts about the year 940.

@viconnex viconnex force-pushed the pc-20049-extra-data branch 6 times, most recently from c57b180 to e96036d Compare January 25, 2023 15:18
@viconnex viconnex marked this pull request as ready for review January 25, 2023 15:29
@viconnex viconnex changed the title (PC-20049)[API] refactor: improve ExtraData (PC-20049)[API] refactor: improve offer ExtraData Jan 25, 2023
Comment on lines +171 to +172
if not self.product_subcategory_id:
raise ValueError("product subcategory id is missing")
Copy link
Contributor

Choose a reason for hiding this comment

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

C'est un bug fix en plus dans ce commit ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Non c'est plutôt une question de typage : une erreur mypy est apparue (parce qu'il y a moins de Any ?). Pour enlever cette erreur mypy, on raise une erreur de manière explicite (de toute façon on aurait eu une KeyError dans ce cas)

api/src/pcapi/core/finance/models.py Outdated Show resolved Hide resolved
api/src/pcapi/core/offers/api.py Outdated Show resolved Hide resolved
api/src/pcapi/core/offers/models.py Show resolved Hide resolved
api/src/pcapi/core/offers/validation.py Outdated Show resolved Hide resolved
@viconnex viconnex force-pushed the pc-20049-extra-data branch from e96036d to e6d8e62 Compare January 25, 2023 16:59
api/src/pcapi/core/offers/validation.py Outdated Show resolved Hide resolved
@viconnex viconnex force-pushed the pc-20049-extra-data branch from e6d8e62 to 7b691dd Compare January 26, 2023 09:14
@viconnex viconnex force-pushed the pc-20049-extra-data branch from 7b691dd to b094f99 Compare January 26, 2023 09:14
Copy link
Contributor

@mgeoffray-pass mgeoffray-pass left a comment

Choose a reason for hiding this comment

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

🍏

@viconnex viconnex merged commit 51f112b into master Jan 26, 2023
@viconnex viconnex deleted the pc-20049-extra-data branch January 26, 2023 09:39
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.

5 participants