-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added support for poll and survey xblocks #4
base: main
Are you sure you want to change the base?
Conversation
Rearranged code to fit in with existing code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR! While I understand the usefulness of polls and surveys, I'd rather limit the number of different unit types, as each unit needs its separate reader/writer for every input/output.
In this context, can we assume that a poll is just a survey with a single question? If we agree on this, then we can have a closer look at the implementation.
As much as possible, you should try to add unit tests to the tests/test_*.py
files. I think you could for instance implement an html reader and an olx writer tests.
Yes, the poll is just 1 question with multiple options but the survey is multiple questions with multiple options where the options are the same for every question. Also, these units do not have any children elements. Instead, they take question(s) and answers as attributes so I had to make a copy of the process_unit function in olx/writer.py to handle this type of unit. |
Right! I noticed that. But my point is that from the perspective of all readers and writers, except for the olx writer, a poll is just a special survey with a single question. So you could really get rid of all the poll-specific code and just implement a different olx writer, which would generate different output based on the number of questions in the survey. Does that make sense? |
Yeah. That does make sense. We can definitely go for this. |
Please let me know when this is ready for review again. |
(FYI I have removed olx.tar.gz from version control, so it should be easier to rebase and resolve conflicts) |
feat: OLX reader for Survey and Poll
@regisb it's ready for review. |
@regisb I have implemented the changes. |
What we tried
What's supported
Limitations
Questions