-
Notifications
You must be signed in to change notification settings - Fork 7
test model dump and fix, add tests to pr workflow #101
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
Conversation
✅ Deploy Preview for processing-contributions ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
fyi, you need to checkout the repo before you can use a custom action, I made the same error a bunch of times |
thanks, I feel a bit better |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved. Left a suggestion
if field in contribution: | ||
if field == 'id': | ||
fh.write(f'{field}={contribution[field]:03}\n') | ||
elif field == 'categories': | ||
if contribution['type'] == 'library': | ||
fh.write(f'{field}={",".join(contribution[field]) if contribution[field] else ""}\n') | ||
else: | ||
# categories are only relevant for libraries, except for examples with "Books" as category | ||
if contribution[field] and 'Books' in contribution[field]: | ||
fh.write(f'{field}={",".join(contribution[field]) if contribution[field] else ""}\n') | ||
else: | ||
fh.write(f'{field}=\n') | ||
elif field == 'compatibleModesList': | ||
fh.write(f'modes={contribution[field]}\n') | ||
else: | ||
fh.write(f'{field}={"" if contribution[field] is None else contribution[field]}\n') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using a match
statement on field
could potentially improve readability. It could also help to put that logic into a write_field()
helper. Just a non-blocking suggestion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll put this comment into an issue, as a possible future improvement to the code.
In recent failed run of contributions update after the additions of latest two contributions, saw that the validation of the properties dictionary and the output of the validation step had some bugs - default values were not exported. Here, have created tests of what we expect, and fixed the data dump. Also moved some parts of script into functions, in anticipation of adding testing.
created pr workflow that runs the tests, and the steps of the update flow.