Refactor blacklist whitelist

mitodl / mitx-grading-library

The MITx Grading Library, a python grading library for edX

https://edge.edx.org/courses/course-v1:MITx+grading-library+examples/

BSD 3-Clause "New" or "Revised" License

14 stars 9 forks source link

Refactor blacklist whitelist #39

Closed ChristopherChudzicki closed 6 years ago

ChristopherChudzicki commented 6 years ago

This PR moves whitelist, blacklist checks in FormulaGrader and IntegralGrader to be performed after the main correctness check:

mathematically correct answers with forbidden functions raise an InvalidInput error declaring the forbidden function
mathematically incorrect answers with forbidden functions are marked incorrect

This brings whitelist/blacklist in-line with the behavior introduced for forbidden_strings in #36.

To facilitate this change, some monolithic FormulaGrader and IntegralGrader functions were refactored into smaller pieces, which is nice.

coveralls commented 6 years ago

Coverage remained the same at 100.0% when pulling b458148f271ca395107554ed272b94ac49d06187 on refactor-blacklist-whitelist into ce7e4a60c1fd9be687bef96a3f043095d907fde1 on master.

ChristopherChudzicki commented 6 years ago

@jolyonb This is ready for review, but no rush. Beyond replying to reviews, I am probably not going to work on the library for a couple of days.

If we merge this, we should let Steppan know.

jolyonb commented 6 years ago

I'm wondering if we can symlink course/static/python_lib.zip to the root directory? We may need to update the script file that creates the edX tarball to follow the symlink appropriately.

jolyonb commented 6 years ago

This looks really good! Two tiny comments and a suggestion for a new PR, I think. I might go and make that other PR now actually...

jolyonb commented 6 years ago

Just made a PR to fix the course zip file. These two PRs are going to have a merge conflict on that file now. I suggest you rebase this PR on top of that one after merging.

ChristopherChudzicki commented 6 years ago

@jolyonb I seem to have screwed up the history after a force push, so... Just see #48 instead.