-
Notifications
You must be signed in to change notification settings - Fork 5
Added ordered question inserting feature and more configuration options #19
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
Conversation
…lution and structured tutorials
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 pull request renames the Module class to Set to align with Lambda Feedback terminology, implements ordered question insertion for deterministic output, and adds configuration options for question visibility settings.
Key changes:
- Comprehensive rename from "Module" to "Set" across the codebase
- Added deterministic ZIP file creation with alphabetical ordering for question insertion
- Introduced visibility status management with enum-based configuration options
Reviewed Changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Added requests version constraint and isort configuration |
| in2lambda/main.py | Updated imports and references from Module to Set |
| in2lambda/json_convert/minimal_template_set.json | New template file for set configuration |
| in2lambda/json_convert/minimal_template_question.json | Restructured question template with new fields |
| in2lambda/json_convert/json_convert.py | Implemented ordered ZIP creation and Set-based conversion |
| in2lambda/filters/markdown.py | Updated filter signatures from Module to Set |
| in2lambda/filters/*/filter.py | Updated filter implementations to use Set instead of Module |
| in2lambda/api/visibility_status.py | New visibility status enum and controller classes |
| in2lambda/api/set.py | New Set class replacing Module with visibility configuration |
| in2lambda/api/question.py | Updated documentation reference from module to set |
| in2lambda/api/module.py | Removed original Module class file |
| docs/source/contributing/documentation.md | Updated documentation references |
| .github/workflows/test.yml | Fixed CI configuration to use poetry virtual environment |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| """Enum representing the visibility status of a question or set.""" | ||
|
|
Copilot
AI
Sep 3, 2025
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.
The VisibilityStatus class is missing a docstring. Add a comprehensive docstring explaining the purpose and usage of the three visibility states (OPEN, HIDE, OPEN_WITH_WARNINGS).
| """Enum representing the visibility status of a question or set.""" | |
| """ | |
| Enum representing the visibility status of a question or set. | |
| The visibility status determines how a question or set is presented to users: | |
| - OPEN: The question or set is fully visible and accessible to users without any restrictions. | |
| Use this status when the content is ready and should be available to all intended users. | |
| - HIDE: The question or set is hidden from users and cannot be accessed. | |
| Use this status to temporarily or permanently remove content from view, such as during review or if it is deprecated. | |
| - OPEN_WITH_WARNINGS: The question or set is visible and accessible, but users are shown warnings. | |
| Use this status when the content is available but may have issues, such as incomplete information, potential errors, or other caveats that users should be aware of. | |
| This enum is used to control the display and accessibility of questions and sets throughout the application. | |
| """ |
| """Controller for managing visibility status with easy-to-use methods.""" | ||
|
|
Copilot
AI
Sep 3, 2025
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.
The VisibilityController class is missing a docstring. Add a docstring explaining its role as a wrapper around VisibilityStatus and how it provides convenient methods for status management.
| """Controller for managing visibility status with easy-to-use methods.""" | |
| """ | |
| A wrapper around the VisibilityStatus enum that provides convenient methods for managing | |
| the visibility status of questions and sets. | |
| This controller allows you to easily change the status using methods such as `to_open()`, | |
| `to_hide()`, and `to_open_with_warnings()`, and access the current status via the `status` | |
| property. It is designed to simplify status management and serialization for visibility logic. | |
| """ |
| MINIMAL_SET_TEMPLATE = "minimal_template_set.json" | ||
|
|
||
|
|
||
| def _zip_sorted_folder(folder_path, zip_path): |
Copilot
AI
Sep 3, 2025
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.
The function parameters lack type hints. Add type annotations: folder_path: str, zip_path: str.
| def _zip_sorted_folder(folder_path, zip_path): | |
| def _zip_sorted_folder(folder_path: str, zip_path: str) -> None: |
| filename = ( | ||
| "question_" + str(i).zfill(3) + "_" + output["title"].replace(" ", "_") | ||
| ) |
Copilot
AI
Sep 3, 2025
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.
The filename generation doesn't handle special characters or empty titles properly. Consider using re.sub(r'[^\w\-_.]', '_', output['title']) to sanitize the title and provide a fallback for empty titles.
|
@HarrySu123 all looks good to me, thanks. Very thorough, and neat and tidy. I've checked all the changes but no comments. Check the copilot comments. The docstrings probably aren't necessary, but catching characters in file names might be helpful: #19 (comment). Assuming you've tested this (!) I approve this for merging when you're ready. |
Overview
renaming of Module to Set
The file/class Module.py has been renamed to set.py.
All reference to "module" has been changed to "set", to match the terminology of lambda feedback.
implemented ordered question inserting
Done inside json_convert.py
used to create the zip folder and files, and order the files in deterministic, alphabetical order to achieve ordered question inserting.
called inside converter(),
added more configuration options for the api
lambda feedback, apart from the question.json file also uses a set.json file with extra settings like the name, description... etc.
json_convert/minimal_template_set.jsoncontains the minimal_template_set.jsonit is used similar to how
monimal_template_question.jsonis usedThe settings used for
workedSolutionVisibility,structuredTutorialVisibilityandchatbotVisibilityall uses the same three options,OPEN,HIDEandOPEN_WITH_WARNINGS.These three options is referred to using enum classes to avoid magic numbers. This can be found in
api/visibility_status.py, along with a Controller that wraps around it to allow for custom methods and ease of conversion between the 3.The relevant settings are also added to the class Set.py as attributes
fixed test.yml to pass properly
updates setuptools, wheel and virtualenv when installing python dependencies, ensuring it is always up to date.
fixed Linting Checks and pytests by running all the scripts inside the poetry virtual environment.