Programming exercises: Add MATLAB programming exercise template
#10039
Conversation
github-actions
bot
added
tests
server
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
magaupp
force-pushed
the
feature/programming-exercises/matlab-template
branch
from
6096857 to
d2c1ba6
Compare
github-actions
bot
added
core
magaupp
force-pushed
the
feature/programming-exercises/matlab-template
branch
from
cc774a5 to
41a9a27
Compare
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmd (7.8.0)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseService.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingLanguageFeatureService.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIProgrammingLanguageFeatureService.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration.
WalkthroughThis pull request introduces comprehensive support for MATLAB as a new programming language in the Artemis platform. The changes span multiple components, including configuration, services, documentation, and templates. Key modifications include adding MATLAB to the list of supported programming languages, creating a license configuration mechanism, updating programming language feature services, and providing MATLAB-specific exercise and test templates. The implementation ensures that MATLAB can be used for programming exercises while respecting its specific limitations and integration requirements. Changes
Suggested Labels
Suggested Reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (15)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java (1)
53-56: Check license conditions for MATLAB feature.The overridden method correctly enumerates the
ProgrammingLanguageFeaturesettings, including MATLAB. However, consider verifying that licensing is enforced in the relevant flow. Presently, the method does not conditionally disable or omit MATLAB if no license is available, which might lead to confusion for instructors/students without a valid license.Also applies to: 68-76
src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIProgrammingLanguageFeatureService.java (1)
33-40: Consider adding MATLAB to the supported features if needed.
You are adding support for MATLAB in the broader PR, but it’s missing in this map. If GitLab CI is intended to support MATLAB, you might want to add a corresponding entry in this method.src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingLanguageFeatureService.java (1)
90-91: License service check is straightforward.
If license checks become more complex in the future, consider caching or advanced logic to prevent repeated lookups.src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseBuildConfigService.java (1)
82-94: Merging environment variables is well-structured.
Be mindful of potential key collisions; the license environment takes precedence.src/main/resources/templates/matlab/exercise/finalGrade.m (1)
1-3: Implement finalGrade logic.
The placeholder is clear, but be sure to define how weighted grades are calculated and return the proper result.Would you like help creating a sample implementation for this function?
src/main/resources/templates/matlab/exercise/averageGradeByStudent.m (1)
1-3: Add the average calculation logic.
The current placeholder lacks an implementation. Please define how to compute and return the average.src/main/resources/templates/matlab/exercise/medianGradeByAssignment.m (1)
1-3: Consider renaming output variable and implementing median calculation logic.The function name
medianGradeByAssignmentdoes not match the output variable namedavg, which may cause confusion. Also, since the function is currently just a placeholder, consider implementing the logic to compute the median by assignment (e.g., using multiple rows or columns ingrades) rather than returning a single median over the entire dataset.src/main/resources/templates/matlab/solution/medianGradeByAssignment.m (1)
1-3: Ensure median is computed for each assignment, and rename output variable.If your intent is to compute the median grade per assignment, consider leveraging the second parameter of MATLAB’s
medianfunction, for example,median(grades, 1)ormedian(grades, 2)depending on your data layout. Also, renaming the output variable fromavgto something more descriptive (e.g.,medGrades) can reduce confusion.src/main/resources/templates/matlab/test/tests/GradeTest.m (1)
6-8: Empty TestClassSetup block
Although it’s perfectly fine to have an emptyTestClassSetupblock, confirm if any shared setup (e.g., path additions, fixture loading, or environment prep) is needed for MATLAB-based testing, especially since MATLAB usage may require licensed toolboxes or specific environment variables.src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationLocalCILocalVCTest.java (1)
248-248: Consider logging for future maintainability
Including a log statement right after this mock line would help maintainers quickly trace the container creation call flow for local CI tests.src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts (1)
251-252: Keep an eye on incorrect feature detection
TheplagiarismCheckSupportedproperty might be undefined for later-introduced languages, especially if no corresponding environment or meta configuration exists. Ensure the rest of the code gracefully handles thefalsefallback without unexpected UI states.src/main/webapp/app/exercises/programming/manage/update/programming-exercise-update.component.ts (1)
365-365: Avoid re-initializing local properties within condition
In your logic forProjectType.MAVEN_BLACKBOXversusPLAIN_MAVEN, repeating the assignment might confuse future maintainers. Consider simplifying with a single assignment or a small helper method that clarifies the allowed transitions.src/main/resources/templates/aeolus/matlab/default.sh (3)
1-2: Enhance script robustness with additional safety flags.Add
-uand-o pipefailflags to catch unset variables and pipeline failures respectively.#!/usr/bin/env bash -set -e +set -euo pipefail
16-20: Pass arguments from main to test function.The main function receives arguments but doesn't pass them to the test function.
main () { - test + test "${@}" }
4-14: Add license validation before MATLAB execution.Given that MATLAB requires a valid license, consider adding license validation before execution.
test () { echo '⚙️ executing test' + # Verify license environment variables + if [ -z "${MATLAB_LICENSE_FILE:-}" ]; then + echo "❌ MATLAB license configuration is missing" + exit 1 + fi + cd "${testWorkingDirectory}"🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 6-6: testWorkingDirectory is referenced but not assigned.
(SC2154)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
src/main/resources/config/application.ymlis excluded by!**/*.ymlsrc/main/resources/templates/aeolus/matlab/default.yamlis excluded by!**/*.yamlsrc/test/resources/config/application.ymlis excluded by!**/*.yml
📒 Files selected for processing (38)
docs/user/exercises/programming-exercise-features.inc(2 hunks)src/main/java/de/tum/cit/aet/artemis/ArtemisApp.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/LicenseConfiguration.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingLanguage.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/LicenseService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseBuildConfigService.java(4 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingLanguageFeatureService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/TemplateUpgradePolicyService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ci/ContinuousIntegrationService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIProgrammingLanguageFeatureService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java(4 hunks)src/main/resources/templates/aeolus/matlab/default.sh(1 hunks)src/main/resources/templates/matlab/exercise/.gitattributes(1 hunks)src/main/resources/templates/matlab/exercise/.gitignore(1 hunks)src/main/resources/templates/matlab/exercise/averageGradeByStudent.m(1 hunks)src/main/resources/templates/matlab/exercise/finalGrade.m(1 hunks)src/main/resources/templates/matlab/exercise/medianGradeByAssignment.m(1 hunks)src/main/resources/templates/matlab/readme(1 hunks)src/main/resources/templates/matlab/solution/.gitattributes(1 hunks)src/main/resources/templates/matlab/solution/.gitignore(1 hunks)src/main/resources/templates/matlab/solution/averageGradeByStudent.m(1 hunks)src/main/resources/templates/matlab/solution/finalGrade.m(1 hunks)src/main/resources/templates/matlab/solution/medianGradeByAssignment.m(1 hunks)src/main/resources/templates/matlab/test/.gitattributes(1 hunks)src/main/resources/templates/matlab/test/.gitignore(1 hunks)src/main/resources/templates/matlab/test/testRunner.m(1 hunks)src/main/resources/templates/matlab/test/tests/GradeTest.m(1 hunks)src/main/webapp/app/entities/programming/programming-exercise.model.ts(1 hunks)src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts(2 hunks)src/main/webapp/app/exercises/programming/manage/update/programming-exercise-update.component.ts(2 hunks)src/main/webapp/app/exercises/programming/shared/service/programming-language-feature/programming-language-feature.service.ts(1 hunks)src/test/java/de/tum/cit/aet/artemis/core/config/LicenseConfigurationTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/service/LicenseServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/util/ArgumentSources.java(2 hunks)src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationLocalCILocalVCTest.java(2 hunks)
✅ Files skipped from review due to trivial changes (6)
- src/main/resources/templates/matlab/solution/.gitattributes
- src/main/resources/templates/matlab/solution/.gitignore
- src/main/resources/templates/matlab/test/.gitignore
- src/main/resources/templates/matlab/test/.gitattributes
- src/main/resources/templates/matlab/exercise/.gitattributes
- src/main/resources/templates/matlab/exercise/.gitignore
🧰 Additional context used
📓 Path-based instructions (21)
src/main/webapp/app/entities/programming/programming-exercise.model.ts (1)
src/test/java/de/tum/cit/aet/artemis/programming/util/ArgumentSources.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingLanguage.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/TemplateUpgradePolicyService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingLanguageFeatureService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/webapp/app/exercises/programming/shared/service/programming-language-feature/programming-language-feature.service.ts (1)
src/main/java/de/tum/cit/aet/artemis/ArtemisApp.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/ci/ContinuousIntegrationService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/config/LicenseConfiguration.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationLocalCILocalVCTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/main/webapp/app/exercises/programming/manage/update/programming-exercise-update.component.ts (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/test/java/de/tum/cit/aet/artemis/core/config/LicenseConfigurationTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIProgrammingLanguageFeatureService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/LicenseService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseBuildConfigService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/test/java/de/tum/cit/aet/artemis/programming/service/LicenseServiceTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
📓 Learnings (3)
src/main/java/de/tum/cit/aet/artemis/programming/service/TemplateUpgradePolicyService.java (1)
Learnt from: magaupp
PR: ls1intum/Artemis#9074
File: src/main/java/de/tum/in/www1/artemis/service/programming/TemplateUpgradePolicyService.java:35-35
Timestamp: 2024-11-12T12:51:46.554Z
Learning: The `defaultRepositoryUpgradeService` in the `TemplateUpgradePolicyService` is used as a default for all non-JAVA languages, with specialized services added based on testing outcomes if necessary.
src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.java (1)
Learnt from: magaupp
PR: ls1intum/Artemis#9256
File: src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.java:0-0
Timestamp: 2024-11-12T12:52:03.805Z
Learning: In `JenkinsProgrammingLanguageFeatureService.java`, comments explaining boolean flags in calls to the `ProgrammingLanguageFeature` constructor are unnecessary because the parameter names are clear, and most IDEs display them as inline hints.
src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIProgrammingLanguageFeatureService.java (1)
Learnt from: magaupp
PR: ls1intum/Artemis#8802
File: src/main/java/de/tum/in/www1/artemis/service/connectors/gitlabci/GitLabCIProgrammingLanguageFeatureService.java:24-24
Timestamp: 2024-11-12T12:51:46.554Z
Learning: Static code analysis for Rust in GitLab CI will be added in a follow-up PR.
🪛 Shellcheck (0.10.0)
src/main/resources/templates/aeolus/matlab/default.sh
[warning] 6-6: testWorkingDirectory is referenced but not assigned.
(SC2154)
🔇 Additional comments (67)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java (4)
14-14: MATLAB import is appropriate.
Importing MATLAB aligns with the new language support. No issues noted with naming or duplication.
30-32: EnumMap and Map usage is consistent.
Using EnumMap<ProgrammingLanguage, ProgrammingLanguageFeature> is optimal for managing a mapping keyed by enum types.
37-38: Dependency import is valid and clear.
Importing LicenseService satisfies the requirement for constructor injection. Good practice to inject the service.
49-51: Constructor injection is well-adhered.
The protected constructor with LicenseService parameter ensures consistent dependency injection, aligning with the "di:constructor_injection" guideline.
src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.java (3)
42-44: Good use of constructor injection
Making the constructor protected here is acceptable if sub-classing is intended. If you want to prevent subclassing and only rely on Spring for instantiation, consider making it public or marking the class as final.
46-47: Method override is clearly defined
The override is well-structured, and the visibility is appropriate. This aligns with the single responsibility principle, as this service specifically provides the mapping of programming languages to their features.
Line range hint 49-66: Missing MATLAB entry and potential duplication
-
The pull request mentions adding MATLAB support, but this method does not include an entry for the MATLAB programming language. Please verify whether MATLAB’s configuration should be part of this map.
-
Each language feature definition has multiple boolean flags, leading to repetitive, verbose code. Consider a more data-driven or builder approach to reduce duplication and make the code easier to maintain.
src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIProgrammingLanguageFeatureService.java (3)
10-12: Imports look good.
No issues found with these added imports.
17-18: Domain and Service imports are appropriate.
These imports align with the domain model and service dependencies.
29-31: Protected constructor ensures controlled instantiation.
Making the constructor protected is valid if you only intend to instantiate or subclass it within the same package or its subclasses.
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingLanguageFeatureService.java (5)
3-7: No issues with new imports.
All added imports appear relevant.
24-31: Constructor injection with LicenseService is sound.
Using constructor injection adheres to recommended DI patterns. However, it’s worth confirming whether protected scope is intentional rather than public for Spring bean creation.
32-32: Abstract method definition aligns with design.
Defining this method abstract enforces subclasses to provide their own feature configurations.
54-55: InfoContributor usage is correct.
Exposing language features through the builder is consistent with the endpoint’s purpose.
57-88: Efficient approach for selectively enabling features.
Using an EnumMap and filtering project types is an effective way to handle features per language.
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseBuildConfigService.java (7)
6-6: HashMap import is fine.
It aligns with storing environment variables as key-value pairs.
21-21: Import for ProgrammingExercise is appropriate.
No issues with referencing the domain.
23-23: ProgrammingLanguage import is consistent with usage.
No issues found.
24-24: ProjectType import is needed for environment logic.
Looks good.
34-38: Constructor injection for LicenseService is consistent.
Matches the pattern of injecting dependencies for licensing checks.
59-80: Conditional logic for Docker flags is clear.
However, if partial flags or invalid network settings appear, ensure the function gracefully handles them.
97-112: Returning null for a missing network and environment might be risky.
Verify that downstream code can handle a null DockerRunConfig. Otherwise, consider returning a default instance.
src/main/resources/templates/matlab/solution/averageGradeByStudent.m (1)
1-3: Validate input dimensions and consider empty inputs.
Although the main logic for computing the average grade by student is sound, consider adding checks for empty or malformed input matrices. For example, handle cases where grades is empty or where the dimensions may be incorrect.
src/main/resources/templates/matlab/solution/finalGrade.m (1)
1-3: Confirm matrix dimensions and consider input validation.
The expression (grades * weights.').' transposes the result. Confirm that the chosen orientation (row vs. column) matches the intended usage. Also, consider validating that grades has a matching number of columns to the length of weights before attempting the multiplication.
src/main/resources/templates/matlab/test/testRunner.m (4)
1-2: Ensure MATLAB integration and paths are accessible
The import and blank line are straightforward, but remember that MATLAB must be installed on the runner environment. Verify that matlab.unittest.plugins.XMLPlugin is available and that the environment can run MATLAB tests without license conflicts.
3-4: Check the directory structure and correctness of the path
Confirm that the ${studentParentWorkingDirectoryName} path exists and is correctly set relative to this script’s location. Incorrect addpath usage can cause missing dependencies.
5-9: Runner and plugin usage look good
This setup for JUnit-compatible XML output is aligned with best practices for reporting. No issues here.
10-12: Finalize or re-check the suite name
Ensure the specified "tests" directory or suite name matches your file structure. If tests are nested in subfolders, consider adjusting the path.
src/main/java/de/tum/cit/aet/artemis/core/config/LicenseConfiguration.java (4)
1-2: Package declaration is correct
The package name matches the project structure for configuration classes.
10-11: Profile and property prefix
Using @Profile(PROFILE_CORE) and @ConfigurationProperties(prefix = "artemis.licenses") is a clean approach to separate license-related configs from other profiles.
14-16: Record usage is concise and appropriate
This record simplifies property handling for the MATLAB license server.
23-29: Nullable return approach
Returning null if the record is null is cautious and safe.
src/test/java/de/tum/cit/aet/artemis/programming/util/ArgumentSources.java (2)
4-4: Import for MATLAB
Explicitly importing MATLAB here is consistent with handling it as a separate case.
18-18: Ensuring MATLAB is in the unsupported list
Since Jenkins does not support MATLAB, adding MATLAB to the unsupported list is correct. This aligns with the PR goal to exclude MATLAB from Jenkins-based pipelines.
src/test/java/de/tum/cit/aet/artemis/core/config/LicenseConfigurationTest.java (3)
9-14: Test with valid license server
This is a straightforward test covering a typical scenario. Good use of assertThat for clarity.
16-20: Null record scenario
Properly tests defense against null in the constructor. This ensures safe default handling.
22-26: Null server value
Good coverage for the edge case where license server is present but null.
src/main/resources/templates/matlab/test/tests/GradeTest.m (3)
1-1: Great initial structure for a MATLAB test class
No issues spotted. The class extends matlab.unittest.TestCase, which is a recommended approach to organize tests in MATLAB.
13-19: Matrix initialization is correct
The grades matrix setup appears correct. If additional logic (e.g., randomized data for robust testing) is planned, consider factoring it into separate helper functions.
25-42: Comprehensive test coverage for grading functions
The three test methods cover median, average, and final grade calculations.
- Confirm that these tests will complete successfully given the placeholder “TODO” in the underlying grade-related functions (if not yet fully implemented).
- The
AbsTol=0.0001tolerance usage is good for floating-point comparisons.
src/main/java/de/tum/cit/aet/artemis/programming/service/TemplateUpgradePolicyService.java (1)
35-37: MATLAB integration with default upgrade service
By grouping MATLAB under the default repository upgrade service, you maintain consistency with other languages. This seems correct.
- Consider verifying that the default upgrade service correctly accommodates any MATLAB-specific logic (e.g., copying
.mfiles, ignoring hidden directories).
src/main/webapp/app/exercises/programming/shared/service/programming-language-feature/programming-language-feature.service.ts (1)
33-34: Allowing undefined return values for unsupported languages
Returning undefined is a safe design choice for missing features. Ensure that calling code handles undefined gracefully, especially since the rest of the code uses the result for UI behaviors.
src/main/java/de/tum/cit/aet/artemis/programming/domain/ProgrammingLanguage.java (1)
50-50: Enabling MATLAB in the supported languages set
Adding MATLAB to ENABLED_LANGUAGES is consistent with the rest of the PR and meets the new feature requirements.
src/test/java/de/tum/cit/aet/artemis/programming/service/LicenseServiceTest.java (5)
20-24: Tests reflect descriptive naming conventions and strong JUnit 5 usage
The test method name is clear, small, and uses assertThat appropriately. This aligns with the test guidelines requiring descriptive naming, small tests, and JUnit 5 features.
26-30: Ensures environment variables are not returned when no license is needed
This test thoroughly validates the service behavior for no-license-needed scenarios, following the principle of small and specific test coverage.
32-36: Properly checks MATLAB licensing requirement
The test ensures licensing logic for MATLAB is recognized. The test remains small and well-scoped, adhering to the coding guidelines.
38-42: Correctly verifies MATLAB environment variable setup
Ensuring that the returned environment map contains the expected MATLAB license key is a crucial test. Nice use of assertThat(environment).containsEntry(...).
44-50: Tests unlicensed MATLAB scenario elegantly
The test demonstrates a clear negative path for license checks, confirming that false is returned. This scenario-based approach is beneficial and meets the small-specific test guideline.
src/main/resources/templates/matlab/readme (4)
1-9: Clear exercise instructions
This section provides concise context about the grading matrix and sets clear expectations for the tasks ahead.
10-21: Well-structured task for median computation
The instructions for computing the median by assignment are sufficiently detailed, guiding students to implement the function properly.
23-33: Appropriate explanation for average grade by student
The textual guide clarifies input-output expectations, helping learners implement consistent logic.
36-49: Comprehensive description for final grade computation
The documentation clearly details the weighted average concept, including how to round results. This is well-organized for a typical MATLAB exercise readme.
src/main/java/de/tum/cit/aet/artemis/programming/service/LicenseService.java (3)
20-21: Scoped Spring bean profile
Annotating the service with @Profile(PROFILE_CORE) ensures that licensing logic is activated only in the intended environment, complying with single responsibility principles.
30-44: Simple and effective license check
The isLicensed method uses a straightforward condition to verify MATLAB license presence. It maintains readability and follows the KISS principle.
46-59: Avoids returning null by providing default environment
The getEnvironment method returns an immutable map, or an empty map when a license is not required. This is a clean approach respecting the best practice of returning empty collections rather than null.
src/main/java/de/tum/cit/aet/artemis/ArtemisApp.java (2)
21-21: License configuration import
Importing LicenseConfiguration is consistent with the new licensing approach. This ensures all license-based properties are recognized by Spring Boot.
28-28: Proper inclusion of LicenseConfiguration in @EnableConfigurationProperties
Registers the properties class alongside other configurations, maintaining a cohesive configuration management pattern in Artemis.
src/main/webapp/app/entities/programming/programming-exercise.model.ts (1)
26-26: Add MATLAB enum entry: Good addition for expanded language support
Adding MATLAB in the ProgrammingLanguage enum appears consistent with the existing naming convention and structure.
src/main/java/de/tum/cit/aet/artemis/programming/service/ci/ContinuousIntegrationService.java (2)
222-223: Include MATLAB in assignment path: Logical extension
Including MATLAB under "assignment" aligns with the approach used for other languages in this switch block.
233-233: Add MATLAB in test path: Maintains consistency
Matching MATLAB to the "tests" directory keeps the structure in sync with similar languages.
docs/user/exercises/programming-exercise-features.inc (2)
50-51: MATLAB in programming language support table
The newly added row correctly notes that MATLAB is supported in Local CI but not Jenkins, matching current platform capabilities.
96-97: MATLAB in feature support table
The row clearly indicates MATLAB’s feature limitations (sequential tests, static analysis, etc.). Documenting these constraints is helpful for instructors.
src/main/java/de/tum/cit/aet/artemis/buildagent/service/BuildJobContainerService.java (1)
111-111: Set entrypoint for improved container command behavior
Switching to .withEntrypoint().withCmd(...) ensures the container’s main process remains active until stop_container.txt is created. This is a solid approach for controlling container lifecycle.
src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationLocalCILocalVCTest.java (1)
257-257: Check chaining consistency
The chained mock calls for .withEntrypoint() and .withCmd(...) look correct here. However, ensure that all other required container properties (like environment variables or volumes) are also appropriately mocked to avoid potential NullPointerExceptions in more complex test setups.
src/main/webapp/app/exercises/programming/manage/programming-exercise-detail.component.ts (1)
211-211: Ensure fallback logic for undefined features
Using optional chaining is good practice. As a next step, consider confirming through unit tests that the fallback (false) is sufficient whenever getProgrammingLanguageFeature returns undefined for certain edge cases (e.g., newly added languages not yet defined).
src/main/webapp/app/exercises/programming/manage/update/programming-exercise-update.component.ts (1)
276-276: Handle potential null from feature lookups
Forcing non-null with ! passes compilation but can lead to runtime errors if the service lookup fails to retrieve a feature. Safeguard by handling or warning if getProgrammingLanguageFeature might return undefined for certain incoming data.
src/main/resources/templates/aeolus/matlab/default.sh (1)
8-10:
Review security implications of sudo operations.
The script performs several privileged operations that could pose security risks:
- Creating directories with sudo
- Changing ownership
- Removing a sudoers file (particularly concerning)
Consider:
- Why does the sudoers file need to be removed?
- Could these operations be performed with less privileged alternatives?
Let's verify the sudoers file usage:
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseService.java
Show resolved
Hide resolved
...n/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseBuildConfigService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingLanguageFeatureService.java (1)
MATLAB integration is incomplete
The codebase has license infrastructure for MATLAB but lacks critical components:
- MATLAB is not defined in the
ProgrammingLanguageenum despite being referenced inLicenseService- No MATLAB-specific feature service implementation exists
🔗 Analysis chain
Line range hint
1-90: Verify MATLAB integration completeness.Since this is part of the MATLAB programming exercise template PR, let's verify that all MATLAB-specific requirements are properly integrated.
Run the following script to check for MATLAB-related configurations and dependencies:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify MATLAB integration completeness # Check for MATLAB-related configurations in application properties echo "Checking for MATLAB license configurations..." rg -l "matlab.*license" --type yaml # Check for MATLAB-related test coverage echo "Checking for MATLAB-related tests..." rg -l "matlab" --type java --glob "*Test.java" # Check for MATLAB programming language enum value echo "Checking for MATLAB programming language definition..." ast-grep --pattern 'enum ProgrammingLanguage { $$$ MATLAB$$$ }'Length of output: 581
Script:
#!/bin/bash # Check for MATLAB enum and related implementations echo "Checking ProgrammingLanguage enum..." rg "enum ProgrammingLanguage" -A 10 echo "Checking license service implementation..." rg "class.*LicenseService" -A 5 echo "Checking MATLAB feature service implementation..." rg "class.*MatlabProgrammingLanguageFeatureService" -A 10Length of output: 4585
Script:
#!/bin/bash # Check LicenseService implementation and configuration echo "Checking LicenseService implementation..." cat src/main/java/de/tum/cit/aet/artemis/programming/service/LicenseService.java echo "Checking LicenseConfiguration..." fd -e java LicenseConfiguration --exec cat {}Length of output: 4290
🧹 Nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java (1)
56-56: Consider enhancing the maintenance comment.The comment "Must be extended once a new programming language is added" could be more helpful.
Consider updating it to:
- // Must be extended once a new programming language is added + // MAINTENANCE: Add new programming languages here. Ensure all ProgrammingLanguage enum values are covered.src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingLanguageFeatureService.java (1)
57-87: Consider refactoring getEnabledFeatures() for better maintainability.The method could benefit from the following improvements:
- Extract the feature filtering logic into a separate method
- Move ProgrammingLanguageFeature construction to a factory method
- Simplify the nested conditionals
Here's a suggested refactoring:
private Map<ProgrammingLanguage, ProgrammingLanguageFeature> getEnabledFeatures() { var features = new EnumMap<ProgrammingLanguage, ProgrammingLanguageFeature>(ProgrammingLanguage.class); for (var entry : getSupportedProgrammingLanguageFeatures().entrySet()) { - var language = programmingLanguageFeatureEntry.getKey(); - var feature = programmingLanguageFeatureEntry.getValue(); - if (feature.projectTypes().isEmpty()) { - if (isProjectTypeUsable(language, null)) { - features.put(language, feature); - } - } - else { - var filteredProjectTypes = feature.projectTypes().stream() - .filter((projectType -> isProjectTypeUsable(language, projectType))) - .toList(); - if (!filteredProjectTypes.isEmpty()) { - var filteredFeature = new ProgrammingLanguageFeature( - feature.programmingLanguage(), - feature.sequentialTestRuns(), - feature.staticCodeAnalysis(), - feature.plagiarismCheckSupported(), - feature.packageNameRequired(), - feature.checkoutSolutionRepositoryAllowed(), - filteredProjectTypes, - feature.auxiliaryRepositoriesSupported() - ); - features.put(language, filteredFeature); - } - } + processFeature(features, entry.getKey(), entry.getValue()); } return features; } +private void processFeature( + Map<ProgrammingLanguage, ProgrammingLanguageFeature> features, + ProgrammingLanguage language, + ProgrammingLanguageFeature feature +) { + if (feature.projectTypes().isEmpty() && isProjectTypeUsable(language, null)) { + features.put(language, feature); + return; + } + + var filteredProjectTypes = filterUsableProjectTypes(language, feature.projectTypes()); + if (!filteredProjectTypes.isEmpty()) { + features.put(language, createFilteredFeature(feature, filteredProjectTypes)); + } +} + +private List<ProjectType> filterUsableProjectTypes( + ProgrammingLanguage language, + List<ProjectType> projectTypes +) { + return projectTypes.stream() + .filter(projectType -> isProjectTypeUsable(language, projectType)) + .toList(); +} + +private ProgrammingLanguageFeature createFilteredFeature( + ProgrammingLanguageFeature original, + List<ProjectType> filteredProjectTypes +) { + return new ProgrammingLanguageFeature( + original.programmingLanguage(), + original.sequentialTestRuns(), + original.staticCodeAnalysis(), + original.plagiarismCheckSupported(), + original.packageNameRequired(), + original.checkoutSolutionRepositoryAllowed(), + filteredProjectTypes, + original.auxiliaryRepositoriesSupported() + ); +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/main/resources/config/application.ymlis excluded by!**/*.yml
📒 Files selected for processing (7)
docs/user/exercises/programming-exercise-features.inc(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingLanguageFeatureService.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIProgrammingLanguageFeatureService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java(3 hunks)src/main/webapp/app/entities/programming/programming-exercise.model.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/webapp/app/entities/programming/programming-exercise.model.ts
- docs/user/exercises/programming-exercise-features.inc
- src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseService.java
🧰 Additional context used
📓 Path-based instructions (4)
src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIProgrammingLanguageFeatureService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingLanguageFeatureService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
📓 Learnings (3)
src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIProgrammingLanguageFeatureService.java (1)
Learnt from: magaupp
PR: ls1intum/Artemis#8802
File: src/main/java/de/tum/in/www1/artemis/service/connectors/gitlabci/GitLabCIProgrammingLanguageFeatureService.java:24-24
Timestamp: 2024-11-12T12:51:46.554Z
Learning: Static code analysis for Rust in GitLab CI will be added in a follow-up PR.
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java (1)
Learnt from: magaupp
PR: ls1intum/Artemis#9261
File: src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java:43-55
Timestamp: 2024-11-12T12:51:40.391Z
Learning: For constructors with multiple boolean parameters, it's acceptable to keep them as is because parameter names are clear and IDEs provide inline hints, making the code readable without refactoring to the builder pattern.
src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.java (1)
Learnt from: magaupp
PR: ls1intum/Artemis#9256
File: src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.java:0-0
Timestamp: 2024-11-12T12:52:03.805Z
Learning: In `JenkinsProgrammingLanguageFeatureService.java`, comments explaining boolean flags in calls to the `ProgrammingLanguageFeature` constructor are unnecessary because the parameter names are clear, and most IDEs display them as inline hints.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: client-style
- GitHub Check: client-tests-selected
- GitHub Check: client-tests
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: Build and Push Docker Image
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
🔇 Additional comments (15)
src/main/java/de/tum/cit/aet/artemis/programming/service/localci/LocalCIProgrammingLanguageFeatureService.java (4)
14-14: LGTM! Import statements are well-organized.The new imports are properly organized and follow the coding guidelines by avoiding star imports.
Also applies to: 30-32, 37-38
49-51: LGTM! Constructor follows best practices.The constructor properly:
- Uses constructor injection for dependencies
- Delegates license management to the superclass
- Follows the principle of least privilege with protected access
53-56: LGTM! Method structure is clean and maintainable.The implementation uses EnumMap for type safety and follows a clear, consistent pattern.
68-68: Verify MATLAB feature configuration.The MATLAB configuration has all features disabled (set to false). Please verify if this is intentional, as it seems to conflict with the PR objective of adding MATLAB support.
Could you clarify the intended feature set for MATLAB? The current configuration might limit its functionality.
src/main/java/de/tum/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.java (3)
26-28: LGTM! Import statements are clean and follow guidelines.The new imports are necessary for the added functionality and follow Java conventions by avoiding wildcard imports.
Also applies to: 33-34
42-44: LGTM! Constructor follows dependency injection best practices.The protected visibility and constructor injection of LicenseService align with Spring best practices and the coding guidelines.
47-67: Verify license-dependent language handling.The PR objectives state that languages requiring a license should be excluded if no license is provided. Please verify that this logic is properly implemented in the superclass.
✅ Verification successful
License-dependent language handling is properly implemented
The superclass
ProgrammingLanguageFeatureServicecorrectly filters programming language features based on license availability through theLicenseService. The system is properly designed to exclude languages that require licenses when they're not provided.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for license-related checks in the superclass ast-grep --pattern 'class ProgrammingLanguageFeatureService { $$$ isLanguageSupported($$$) { $$$ } $$$ }'Length of output: 243
Script:
#!/bin/bash # Find the superclass file fd -e java ProgrammingLanguageFeatureService # Search for license-related checks rg -l "license" --type java # Search specifically for language support checks rg "isLanguageSupported|requiresLicense" --type java -A 5Length of output: 1496
Script:
#!/bin/bash # View the superclass implementation cat src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingLanguageFeatureService.java # View the license service implementation cat src/main/java/de/tum/cit/aet/artemis/programming/service/LicenseService.javaLength of output: 6768
src/main/java/de/tum/cit/aet/artemis/programming/service/gitlabci/GitLabCIProgrammingLanguageFeatureService.java (4)
10-12: LGTM! Import statements are well-organized.The new imports are properly scoped and follow Java coding guidelines by avoiding star imports.
Also applies to: 17-18
29-31: LGTM! Constructor follows dependency injection best practices.The constructor properly:
- Uses constructor injection as per Spring best practices
- Passes the LicenseService to the superclass
- Has protected visibility which is appropriate for a deprecated class
Line range hint
23-24: LGTM! Clear deprecation notice with migration path.The deprecation notice properly indicates:
- When the feature will be removed (8.0.0)
- When it was deprecated (7.5.0)
- A clear migration path with a reference PR
33-40: 🛠️ Refactor suggestionAdd MATLAB support and improve method implementation.
The method implementation has several areas for improvement:
- Consider making the method final since the class is deprecated
- MATLAB support is missing despite being a key objective of this PR
- Return an unmodifiable map for thread safety
Apply this diff to address these points:
@Override - protected Map<ProgrammingLanguage, ProgrammingLanguageFeature> getSupportedProgrammingLanguageFeatures() { + protected final Map<ProgrammingLanguage, ProgrammingLanguageFeature> getSupportedProgrammingLanguageFeatures() { EnumMap<ProgrammingLanguage, ProgrammingLanguageFeature> programmingLanguageFeatures = new EnumMap<>(ProgrammingLanguage.class); programmingLanguageFeatures.put(EMPTY, new ProgrammingLanguageFeature(EMPTY, false, false, false, false, false, List.of(), false)); programmingLanguageFeatures.put(JAVA, new ProgrammingLanguageFeature(JAVA, false, false, false, true, false, List.of(PLAIN_MAVEN, MAVEN_MAVEN), false)); programmingLanguageFeatures.put(JAVASCRIPT, new ProgrammingLanguageFeature(JAVASCRIPT, false, false, true, false, false, List.of(), false)); programmingLanguageFeatures.put(RUST, new ProgrammingLanguageFeature(RUST, false, false, true, false, false, List.of(), false)); + programmingLanguageFeatures.put(MATLAB, new ProgrammingLanguageFeature(MATLAB, false, false, true, false, false, List.of(), true)); - return programmingLanguageFeatures; + return Map.copyOf(programmingLanguageFeatures); }Let's verify the MATLAB configuration in other files:
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingLanguageFeatureService.java (4)
3-7: LGTM! Good choice using EnumMap and Nullable annotation.The switch to EnumMap is more efficient for enum keys, and the Nullable annotation improves null safety.
Also applies to: 14-14
24-31: LGTM! Well-structured dependency injection and immutable fields.The implementation follows Spring best practices with constructor injection and immutable fields, promoting thread safety.
54-55: LGTM! Simplified contribute method.The direct use of map values makes the code more concise and efficient.
89-90: LGTM! Clean delegation to LicenseService.The method is concise, properly annotated, and follows the single responsibility principle.
...um/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.java
Show resolved
Hide resolved
...um/cit/aet/artemis/programming/service/jenkins/JenkinsProgrammingLanguageFeatureService.java
Show resolved
Hide resolved
|
Merge included the removal of the testwise coverage report flag in |
vinceclifford
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
Checklist
General
Server
Changes affecting Programming Exercises
Motivation and Context
MATLAB requires a license to be run.
Description
This PR adds a MATLAB programming exercise template.
A
LicenseServiceis added which provides environment variables with licensing information where required.Licenses are configured in the
application-*.ymlfiles:The
ProgrammingExerciseBuildConfigServiceis updated to include these environment variables in its generatedDockerRunConfig.Docker container instantiation in
BuildJobContainerServiceis updated to explicitly clear theENTRYPOINTwhich results in more predictable behavior with itsCMD. This is required if the docker image uses aENTRYPOINTwhich does not run its ´CMD` in a shell.The
ProgrammingLanguageFeatureServices are updated to remove languages or project types which require a license if none is provided.Steps for Testing
Prerequisites:
Testing locally requires a reachable license server.
TUM students may use the license server within the faculty network.
To find its address and port connect to lxhalle via SSH and read the
/usr/applic/packages/matlab/licenses/network.licfile.The license server is reachable via eduVPN or within eduroam.
Configure the license server in
application-local.ymlas shown above.MATLABas the programming languageTestserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Test Coverage
Server
Summary by CodeRabbit
Summary by CodeRabbit
Here are the release notes for this pull request:
New Features
Documentation
.gitattributesand.gitignorefiles for MATLAB projects.Configuration
Testing