Extended Hinting #4788
Extended Hinting #4788
Conversation
There was a problem hiding this comment.
General Notes about this test file:
- The new hinting mechanism introduces to the XML format a 'schema' attribute on the element. Thus, with every XML string produced by the new code, this new attribute must be included with the proper value ('edXML/1.0' at the time of the original PR). The tests in this file were all modified to expect both the new attribute and this initial schema value. This change is self-evident so the changed lines have not been annotated.
- These tests, as written, are somewhat fragile in that a simple string comparison is performed between the actual resulting XML string against the expected XML string. XML parsers are designed to ignore nearly all whitespace--two XML strings can differ wildly in their use of whitespace and still be exactly equivalent in terms of the XML content of each. Considerable time was spent tweaking the new XML generation code to produce exactly the same XML strings so that the regression tests here would not need editing. Unfortunately, in some cases the cost of matching the expected string verbatim was deemed too great and this test file was changed instead. The justification in each case is the same: whitespace may be removed or added to an XML string without changing the XML content in any substantive way. The removal of whitespace should be self-evident in the diff below so those lines have not been annotated.
There was a problem hiding this comment.
this test, as written, was invalid markdown. a new case was added to make each answer the only answer associated with a single component.
|
@DavidAdams and @NickParlante would you mind taking a look at this PR when you have a chance? |
|
Hey Thomas -- woah, this thing is kind of big! I'm checking out capa a bit, and just typing in what pops to mind. If I write "I wonder if xyz" that means just think about it as an alternative, not like something I'm insisting on. @trbm |
|
@nparlante how (if at all) does this PR relate to the hinting facilities that were eventually added with #1499 ? |
common/lib/capa/capa/inputtypes.py
Outdated
There was a problem hiding this comment.
Hey, I like these nice long comments! I wonder if this method could be titled more along the lines of "covert to pre 1.0 options". The current title seems a little zoomed in.
I don't know that we use _xyz method naming much on this project, so you I would think about that. Like anyone who reads the comment can see that this is not some general-purpose method to call at will, and then we leave it up to them.
There was a problem hiding this comment.
I believe it is customary to leave the init as the first method of a class.
|
@sarina one of the main objectives of this 'new hinting' effort was to rationalize the way hints/targeted feedback is added to these common components. as we (OLI at Stanford) starting looking into what mechanisms already existed, we found there are several different ways implemented to solve the same problem. we were careful not to break any of the old desultory ways of handling hints but add a consistent syntax (for markdown) and XML representation in the hope that this new way would be easier to maintain and for course authors to learn. |
There was a problem hiding this comment.
@trbm Hey Thomas -- nice implementation notes! It would be interesting to take a look at how the "1499" stuff got it's stuff into the get_html pipeline. I don't recall it being especially cringe inducing.
There was a problem hiding this comment.
Please add unit tests for this function, it is untested
There was a problem hiding this comment.
L686 in this file is also untested (where this function is called), so it would be good to have unit tests that hit that line and called this function.
common/lib/capa/capa/inputtypes.py
Outdated
There was a problem hiding this comment.
Can you explain why you are only considering the first element in the list. //problem selects all problem sub-elements in the tree so could there be more than one?
There was a problem hiding this comment.
this is actually a lookup of the single root element of the xml tree:
i modified the code to use '/problem' rather than '//problem' to clarify that and added some comments as well
|
Unit test coverage looks a bit patchy: common/lib/capa/capa/inputtypes.py (93.3%): Missing line(s) 527,533 |
|
@dcadams I have addressed all concerns arising from your latest review. When you are satisfied that your review process can be closed, please add a note here to that effect for the record. Thanks. |
|
@trbm I am waiting for a clean test run before starting to review. Have you rebased on master recently? I think that will be necessary to resolve the Firefox issue. You may find it easier to create a new branch off of master and then cherry-pick your commit. This will result in a new PR, but you can provide a link to this first PR. |
|
He @trbm it looks like you've done something wonky here with this PR. You should rebase atop master, NOT merge master into your branch. Somehow you've picked up a bunch of commits that aren't yours. Please see https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request . If you have trouble, see if there's someone at Stanford who wouldn't mind sitting over your shoulder and walking you through it - that can make it easier the first few times. |
|
@trbm This PR is still showing "extra" commits. Please add a comment when the PR is in shape for continuing review. |
|
@cahrens @sarina Hey Christina and Sarina -- I had good talk with Thomas, getting a bigger picture understanding of how this thing works. The plan is I'm going to review and iterate with him for a bit, working on your comments and general cleanup. We'll ping you when that's done. In particular Thomas and I talked about the following simplification which I'm hoping an be made to work: Currently the early stages insert "tokens", which flow through the current xml generation, and then in a later stage get replaced by XML. I'm hoping for a more direct-to-xml approach, where the markup goes to xml without tokens, like we do everything else. That has some tricky bits, but would cut out all the token code, so Thomas and I talked about trying to make that work. The first challenge is fiddling with the .coffee parsing for a case like the following. I think this can be done with some regex powerups. (x) An option {{ here is some extended hint text }} |
|
Thanks for the pull request, @trbm! I've created OSPR-94 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams. Supporting information that is good to add there is anything that can help Product understand the context for the PR - supporting documentation, edx-code email threads, timeline information ('this must be merged by XX date', and why that is), partner information (this is for a course on edx.org, for example), etc. All technical communication about the code itself will still be done via the Github pull request interface. As a reminder, our process documentation is here. We can't start reviewing your pull request until you've added yourself to the AUTHORS file. Please see the CONTRIBUTING file for more information. |
openedx-webhooks
added
the
waiting on author
…nto trbm/hints_PR Conflicts: common/lib/xmodule/xmodule/js/src/problem/edit.coffee
|
@nparlante are you going to continue work on this PR, or open up a new one? |
|
Hey Sarina -- yes, I'm working on it. On 10/14/14 9:42 AM, Sarina Canelake wrote:
|
|
@nparlante yeah, I think it makes sense for me to close this PR and for you to submit a new one. We'll still have the context of this one as reference, but it is already super lengthy (~200 comments). And, as you mention, you're now working on the fork model, so it will be hard/impossible for you to actually continue work on this branch. |
|
Once everything is merged, I'll delete the branch. But not going to delete it for now. |
https://openedx.atlassian.net/browse/TNL-34
This pull request represents an implementation for a new hint (aka targeted feedback) mechanism designed to provide a coherent approach to hinting in all of the five most frequently used problem types: checkboxes, multiple choice, drop down, numeric input, and text input. There are four parts to this new mechanism:
the documentation describing how the new hint material will be encoded in the XML representation of problems (see https://docs.google.com/document/d/1h1cWx24VWkYCdwQEV8rY2Jm1xcZZp6_EQQbzDqbrnks/edit#heading=h.obezscv16esk),
a description of the new syntax added to the simple editor's markdown parser to allow a user of that tool to capture the hinting information in a way which will produce correct XML (see the link above),
the code changes which were made to the markdown parser (see problem/edit.coffee),
the code changes which were made to the response processing to extract the new hint material from the XML representation and add it to the html presented to the student (see responsetypes.py).