Skip to content

Conversation

@HarrySu123
Copy link
Collaborator

Overview

  • renaming of Module to Set
  • implemented ordered question inserting
  • added more configuration options for the api
  • added relevant and correct python docstring where needed
  • fixed test.yml to pass properly

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

def _zip_sorted_folder(folder_path, zip_path):
    """Zips the contents of a folder, preserving the directory structure.

    Args:
        folder_path: The path to the folder to zip.
        zip_path: The path where the zip file will be created.
    """
    with zipfile.ZipFile(zip_path, "w") as zf:
        for root, dirs, files in os.walk(folder_path):
            # Sort files for deterministic, alphabetical order
            for file in sorted(files):
                abs_path = os.path.join(root, file)
                rel_path = os.path.relpath(abs_path, folder_path)
                zf.write(abs_path, arcname=rel_path)

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.json contains the minimal_template_set.json
it is used similar to how monimal_template_question.json is used

The settings used for workedSolutionVisibility, structuredTutorialVisibility and chatbotVisibility all uses the same three options, OPEN, HIDE and OPEN_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.

@HarrySu123 HarrySu123 requested a review from Copilot September 3, 2025 16:19
Copy link

Copilot AI left a 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.

Comment on lines +6 to +7
"""Enum representing the visibility status of a question or set."""

Copy link

Copilot AI Sep 3, 2025

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).

Suggested change
"""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.
"""

Copilot uses AI. Check for mistakes.
Comment on lines +22 to +23
"""Controller for managing visibility status with easy-to-use methods."""

Copy link

Copilot AI Sep 3, 2025

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.

Suggested change
"""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.
"""

Copilot uses AI. Check for mistakes.
MINIMAL_SET_TEMPLATE = "minimal_template_set.json"


def _zip_sorted_folder(folder_path, zip_path):
Copy link

Copilot AI Sep 3, 2025

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.

Suggested change
def _zip_sorted_folder(folder_path, zip_path):
def _zip_sorted_folder(folder_path: str, zip_path: str) -> None:

Copilot uses AI. Check for mistakes.
Comment on lines 99 to 101
filename = (
"question_" + str(i).zfill(3) + "_" + output["title"].replace(" ", "_")
)
Copy link

Copilot AI Sep 3, 2025

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.

Copilot uses AI. Check for mistakes.
@peterbjohnson
Copy link
Contributor

@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.

@HarrySu123 HarrySu123 merged commit 9abc753 into main Sep 10, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants