Allow hints to generate content errors - #827
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #827 +/- ##
==========================================
+ Coverage 41.08% 41.68% +0.59%
==========================================
Files 550 550
Lines 23629 23687 +58
Branches 2855 2869 +14
==========================================
+ Hits 9709 9874 +165
+ Misses 13044 12918 -126
- Partials 876 895 +19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
sjd210
left a comment
There was a problem hiding this comment.
I think this is fine. We already don't rely on the content errors as a perfect list of things that are wrong with the content, and anything weird that this could introduce (either now, or with new errors in the future) is pretty explicitly an odd case that should be treated as exceptional.
This works for what we want it for, so I think its good! 👍
sjd210
left a comment
There was a problem hiding this comment.
I still think this is fine in principle, but just spotted something for before this gets merged.
| setOfContentObjects.addAll(flattenContentObjects((Content) child)); | ||
| } | ||
|
|
||
| if (content instanceof IsaacQuestionBase question) { |
There was a problem hiding this comment.
Hints don't just exist on a Question, they can also be part of an InlineRegion - so we ought to also add this logic to the if statement below this as well. e.g. See sjd_inline_region_no_questions
There was a problem hiding this comment.
Good spot! I've also aligned the inlineQuestions loop to include a null-check, just in case.
I would appreciate some discussion on whether this is the right place to do this. This modifies the unpacking inside
flattenContentObjectsto include hints, alongside children and inline question regions. This seems to work fine, but does mean the ETL process considers more objects than before and now, for example, may not throw a content error if the only question part in a question is located inside a hint. The editor of course forbids this and other silly edge cases, so I am not worried by just this – but there may be more problematic cases that I have missed.