Dev4X / oppia

Automatically exported from code.google.com/p/oppia
Apache License 2.0
0 stars 0 forks source link

Code review request #874

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
Branch name:
implement-fuzzy-rules

Link to the relevant commit(s):
https://code.google.com/p/oppia/source/detail?r=69f73ae095043f452f8042d62019b2b8
2710e692&name=implement-fuzzy-rules

Purpose of code changes on this branch:
To introduce fuzzy rules to the rule backend and training interfaces to the 
exploration editor.

When reviewing my code changes, please focus on:
Everything, but in particular:

1. Should we try and present the answers in a better way? It is now using the 
rendered HTML templates that the learner view uses, but it is still a bit 
awkward.
2. Should the training modals and panels be separated from StateResponses?
3. What other tests might be helpful to add? I made sure that the changes to 
exp_domain were tested in both exp_domain_test and exp_services_test. I also 
made sure that the query potential training data handler was properly handled.
4. Should we interpolate parameters within feedback? E.g., it will currently 
show {{answer}} in the training panel rather than pasting in what the actual 
answer is.
5. In what ways could the training and test modals be cleaned up?
6. The train and test buttons do not have drop shadows--should they?
7. What do you think about multiple answer groups having no feedback? This will 
result in some confusion when trying to determine which answer group should be 
selected for a particular answer during training.
8. Should 'FuzzyMatches' be defined in feconf?

After the review, I'll merge this branch into: develop

Original issue reported on code.google.com by bhenn...@google.com on 23 Jul 2015 at 4:48

GoogleCodeExporter commented 9 years ago
This looks very good overall! With regards to your questions:

(1) I think this is fine for now.
(2) Up to you.
(3) Test coverage seems reasonable; I commented in cases where I thought some 
could be added.
(4) It would be nice to do this, but it's also OK to leave it for later if you 
don't have bandwidth.
(5, 6) Please ask Amit (cc'd).
(7) I picked up on that in the review too, and I don't like it. Can we perhaps 
disambiguate by destination in that case?
(8) Yes, or somewhere sensible.

Original comment by s...@google.com on 24 Jul 2015 at 7:50