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

fix parsing of partnerships #187

Merged
merged 5 commits into from
Jul 12, 2024

Conversation

sneakers-the-rat
Copy link
Contributor

Fix: #186

sorry this is coming so late today, i have been helping everyone else will stuff all day long and haven't even gotten to any of my own work yet lmao, but since i caused this bug i wanted to fix ASAP.

so here's fix as discussed

  • model - used string enum (python 3.10 doesn't have enum.StrEnum) instead of a Literal because in the future it would be nice to be able to add annotations like the text that comes after the key in the template currently so that we can generate the review template directly from the model. It does the same thing - the pydantic model will validate and throw an error there if there's any value that isn't in that enum.
  • get_categories - since the same method get_categories is used for parsing a regular checklist and a checklist that is effectively a key: value pair, i added a keyed param that extracts the key.
  • test, regex - to do that, i made a regex pattern and added tests for that so at least i have declared the expected behavior of that pattern lol
  • test, parsing - also added a fake review based on a real one to test actual parsing of an actual issue

so by making the model more strict we now can make the claim that "this key will always be this value or absent" and "if the string is within the tested boundaries of the regex pattern it will be present." We need to do that for the rest of the model now too - make it reflect the contract we want to make with anyone ingesting data from our packages.yaml file: eg. currently package_name is optional and defaults to a blank string, but we should actually never have a review with no package name. and so on. but that's enough for this v simple lil bug

@lwasser
Copy link
Member

lwasser commented Jul 12, 2024

@sneakers-the-rat thank you for this!! it works perfectly. i'm going to merge now.

@lwasser lwasser merged commit 1033fe1 into pyOpenSci:main Jul 12, 2024
2 checks passed
@lwasser
Copy link
Member

lwasser commented Jul 12, 2024

you are also correct there are a few please where we could impose a Literal // set of accepted values. And also not accept a missing field such as package name and repo url (authors / maintainers too).

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.

bug: Problem with how partners field is parsed + tests
2 participants