Skip to content
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

feat: Add warning for unrecognized elements #7983

Closed

Conversation

eliotwrobson
Copy link
Collaborator

See title, resolves #2279 for the experimental renderer. Not tested yet.

Also, not sure if it's worth adding this to the old render code either. It's nice to have new warnings like this behind a feature flag.

@eliotwrobson eliotwrobson added the python Requires Python code changes label Jun 22, 2023
@eliotwrobson eliotwrobson self-assigned this Jun 22, 2023
@eliotwrobson eliotwrobson removed their assignment Jul 18, 2023
@eliotwrobson eliotwrobson self-assigned this Oct 16, 2023
@nwalters512
Copy link
Contributor

I think if we're going to do this, we should do it for both the experimental and standard renderer at the same time.

I'm a little worried about rolling this out, but I do think it's a good enough of a feature to get out there. I think it'd be preferable to have explicit issues created than allowing mis-named elements to go undetected. Let's think about how to do this safely. Maybe we write a little script to crawl all currently-existing questions and look for cases that this would catch? Or we could run this in log-only mode for a while and see if this is catching things; if not, we can reason that it's probably safe to turn on.

I do think putting this behind a global feature flag would be a good idea so that if people do start complaining, we can quickly shut it off. Alternatively, we could do what we do for R and use a reverse feature flag of sorts. That is, if question-allow-unrecognized-elements is enabled for a course, we skip the check.

@eliotwrobson do you want to queue this up for discussion at the next dev meeting?

@eliotwrobson
Copy link
Collaborator Author

I think if we're going to do this, we should do it for both the experimental and standard renderer at the same time.

In it's current form yes, though the more I think about it (thoughts in the later responses), this feature as currently conceived is less useful than it could be if paired with some other features targeted at developer support. I think we should also have a discussion about the possibly limited rollout of the experimental renderer, since that has the potential to support more developer-focused features than the standard one (along with just being faster), and maintaining both at once seems like more effort than it's worth if the plan is to move to something resembling the experimental one eventually.

I'm a little worried about rolling this out, but I do think it's a good enough of a feature to get out there. I think it'd be preferable to have explicit issues created than allowing mis-named elements to go undetected. Let's think about how to do this safely. Maybe we write a little script to crawl all currently-existing questions and look for cases that this would catch? Or we could run this in log-only mode for a while and see if this is catching things; if not, we can reason that it's probably safe to turn on.

I originally had the same thought, but the issue is that there may be questions that depend on mis-named elements being a noop, and more than likely, any question that has been deployed by instructors with a mis-named element has been tested for following expected behavior. Since questions with this issue currently have expected behavior, raising an issue that gets shown to the student is probably more of an annoyance than a help for most instructors. I also don't think it's possible to have a script crawl questions, since the rendering effectively has to be performed to catch this error (most of the misnamed elements are probably in markup that gets generated in other places).

Although I still agree that this is a useful feature, especially for new users and during question development, I think the best way to roll this out would actually be with #7337, where the issues created are not shown to the student.

@/eliotwrobson do you want to queue this up for discussion at the next dev meeting?

Sure, I can collect some ideas from the above and other issues that reference warnings that would be useful to show course content developers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Requires Python code changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Throw an error on unrecognized pl-* elements
2 participants