-
Notifications
You must be signed in to change notification settings - Fork 56
Language options #1247
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
Language options #1247
Conversation
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.
Looks fine to me. Please format and fix any other failing CI
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.
LGTM, thanks!
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.
Pull Request Overview
This PR adds support for language options in the programming language configuration by introducing a new language_options field to the Library schema. The changes enable parsing language-specific configuration options from XML and exposing them through the API.
- Added
language_optionsfield to the Library embedded schema with a default value of an empty map - Updated XML parser to extract OPTION elements from PROGRAMMINGLANGUAGE tags and convert them to a key-value map
- Refactored XML tag names from DEPLOYMENT/GRADERDEPLOYMENT to PROGRAMMINGLANGUAGE/GRADERPROGRAMMINGLANGUAGE and renamed the parsing function accordingly
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/cadet/assessments/library.ex | Added language_options field to Library schema and included it in optional fields for validation |
| lib/cadet/jobs/xml_parser.ex | Updated XML parsing to use PROGRAMMINGLANGUAGE tags, renamed parsing function, and added logic to extract OPTION elements into language_options map |
| lib/cadet_web/helpers/assessments_helpers.ex | Added languageOptions mapping to expose the new field in API responses |
| test/cadet_web/admin_controllers/admin_grading_controller_test.exs | Updated test expectations to include the new languageOptions field in library objects |
Comments suppressed due to low confidence (1)
lib/cadet/assessments/library.ex:32
- The new
language_optionsfield lacks validation similar to theglobalsfield. Sincelanguage_optionsis a map that will be parsed from XML attributes and exposed in the API, it should have validation to ensure the map contains only string keys and string values, similar tovalidate_globals/1. Consider adding avalidate_language_options/1function and calling it in the changeset pipeline.
def changeset(library, params \\ %{}) do
library
|> cast(params, @required_fields ++ @optional_fields)
|> cast_embed(:external)
|> put_default_external()
|> validate_required(@required_fields ++ @required_embeds)
|> validate_globals()
|> validate_chapter()
|> validate_chapter_variant()
end
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@sentry generate-test |
|
Sentry has determined that unit tests are not necessary for this PR. |
|
@sentry review |
This PR aims to add language options to Source Academy missions. The use case for now is allowing instructors to configure type strictness in Source Typed variants.
Library Schema
The
LibrarySchema is used to describe the language configuration for an assessment question. An optional fieldlanguageOptionis added into theLibraryschema to achieve our objective.XML Parsing
This changes the XML parser. Specifically, the XML tags
<DEPLOYMENT>and<GRADERDEPLOYMENT>are renamed to<PROGRAMMINGLANGUAGE>,<GRADERPROGRAMMINGLANGUAGE>, as per discussion.<LANGUAGEOPTIONS>tags are also added to configure programming language flags.Example usage of
<PROGRAMMINGLANGUAGE>,<GRADERPROGRAMMINGLANGUAGE>and<LANGUAGEOPTIONS>tag:These changes are not breaking and should work with existing assessments. For future assessments, XMLs have to be changed.