draft: sketched out SO interface CI workflow.#798
Conversation
github-actions
bot
added
the
core
|
I just sketched out how this could work, but this could be a project for Andy. |
tristpinsm
left a comment
tristpinsm
left a comment
There was a problem hiding this comment.
This is really starting to take shape! I took a quick look and left a few comments, but I can give it a closer look later on and once we've had a chance to discuss. Have you tried running this on the code?
tristpinsm
left a comment
tristpinsm
left a comment
There was a problem hiding this comment.
Added some more comments to the code.
tristpinsm
left a comment
tristpinsm
left a comment
There was a problem hiding this comment.
Here's a first round of review :)
tristpinsm
left a comment
tristpinsm
left a comment
There was a problem hiding this comment.
Hi Andy, this is looking great! I think functionally this is all good, but I left some more suggestions and comments that I think could improve the clarity and usability of the code. This is maybe a bit of a pedantic exercise, but hopefully helps you hone those best practices.
Let me know if you have any questions, and definitely if you disagree with any of the suggestions.
tests/ci/check_so_interface.py
Outdated
| if check == "incorrectdefaults": | ||
| raise Exception("Mismatch for defaults in function "+node.name+" occurred in file: "+fname) | ||
| try: | ||
| assert compare_args(child, interface[fname][node.name][node.name]) |
There was a problem hiding this comment.
there should probably be just one indexing of node.name here?
tristpinsm
left a comment
tristpinsm
left a comment
There was a problem hiding this comment.
Noted a few more small fixes to make, but then we should be good to merge.
tests/.DS_Store
Outdated
There was a problem hiding this comment.
remove this before we merge
tests/ci/check_so_interface.py
Outdated
| # in principle, we could have functions defined in the body (though I don't think we do in this case) | ||
| if node.name in interface[fname]: | ||
| try: | ||
| assert compare_args(child, interface[fname][node.name][node.name]) |
There was a problem hiding this comment.
there should probably be just one indexing of node.name here?
tests/.DS_Store
Outdated
There was a problem hiding this comment.
this file is still here...
|
@swh76 I can't approve this PR because I was originally the author, but I think it looks good to go. If you'd like to take a look, feel free, but we'll need your approval to merge (squash merge I think in this case). |
2 participants
Description
Draft of github CI workflow to enforce compliance with the SO interface document here.
This is just a sketch and the code needs to be fleshed out, tested and documented.