feat: add export_languages support in the config file#985
feat: add export_languages support in the config file#985katerina20 wants to merge 1 commit intomainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #985 +/- ##
============================================
+ Coverage 65.30% 65.41% +0.12%
- Complexity 1679 1686 +7
============================================
Files 244 244
Lines 6881 6886 +5
Branches 1046 1047 +1
============================================
+ Hits 4493 4504 +11
+ Misses 1790 1786 -4
+ Partials 598 596 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds export_languages support to the CLI configuration so downloads can be restricted to a configured subset of target languages (optionally further filtered by excludeLanguageIds).
Changes:
- Introduces
export_languagesconfig key and maps it ontoPropertiesWithFiles. - Updates
DownloadActionto compute target languages using CLI args,export_languages, and excluded languages. - Adds a
DownloadActionTestcovering theexport_languages+ exclude-languages interaction and extends the test properties builder API.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/crowdin/cli/properties/PropertiesBuilder.java | Adds EXPORT_LANGUAGES config key constant. |
| src/main/java/com/crowdin/cli/properties/PropertiesWithFiles.java | Adds exportLanguages field and populates it from config. |
| src/main/java/com/crowdin/cli/commands/actions/DownloadAction.java | Uses export_languages (when no --language args) to decide build target languages; refactors language resolution into a helper. |
| src/test/java/com/crowdin/cli/properties/NewPropertiesWithFilesUtilBuilder.java | Adds setExportLanguages(...) to simplify test setup. |
| src/test/java/com/crowdin/cli/commands/actions/DownloadActionTest.java | Adds a test case for configured export languages with excluded languages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public NewPropertiesWithFilesUtilBuilder setExportLanguages(List<String> exportLanguages) { | ||
| this.pb.setExportLanguages(exportLanguages); | ||
| return this; | ||
| } |
There was a problem hiding this comment.
setExportLanguages(...) mutates the underlying PropertiesWithFiles, but buildToString() (used to generate a config file in tests) doesn’t serialize export_languages. This makes the builder inconsistent and will silently drop the setting when someone uses setExportLanguages(...).buildToString() to craft a config-based test. Consider updating buildToString() to include export_languages when present (and ensure formatting matches the existing config text conventions).
| public void testProjectFittingFile_ExcludeLanguagesOnly() throws IOException, ResponseException { | ||
| NewPropertiesWithFilesUtilBuilder pbBuilder = NewPropertiesWithFilesUtilBuilder | ||
| .minimalBuiltPropertiesBean("*", Utils.PATH_SEPARATOR + "%original_file_name%-CR-%locale%") | ||
| .setBasePath(project.getBasePath()) | ||
| .setExportLanguages(List.of("de", "ua")); |
There was a problem hiding this comment.
The test name testProjectFittingFile_ExcludeLanguagesOnly is misleading: the setup also relies on export_languages from the config and verifies the interaction between export_languages and excludeLanguageIds. Renaming the test to reflect both inputs would make failures easier to interpret.
| .map(FileBean.CONFIGURATOR::buildFromMap) | ||
| .collect(Collectors.toList())); | ||
| props.setPseudoLocalization(PseudoLocalization.CONFIGURATOR.buildFromMap(PropertiesBuilder.getMap(map, PSEUDO_LOCALIZATION))); | ||
| PropertiesBuilder.setPropertyIfExists(props::setExportLanguages, map, EXPORT_LANGUAGES, List.class); |
There was a problem hiding this comment.
export_languages is loaded as a raw List via setPropertyIfExists(..., List.class) and assigned to a List<String> field. If the config contains non-string items (e.g., numbers), this will pass the type check but later fail with a ClassCastException when iterating in DownloadAction. Consider adding a dedicated helper to validate/convert a list of strings (similar to getListOfMaps) or explicitly validating elements here to throw a ValidationException with a clear message.
| PropertiesBuilder.setPropertyIfExists(props::setExportLanguages, map, EXPORT_LANGUAGES, List.class); | |
| Object exportLanguagesObj = map.get(EXPORT_LANGUAGES); | |
| if (exportLanguagesObj instanceof List<?>) { | |
| List<?> rawList = (List<?>) exportLanguagesObj; | |
| List<String> validatedExportLanguages = new ArrayList<>(rawList.size()); | |
| for (Object item : rawList) { | |
| if (!(item instanceof String)) { | |
| throw new IllegalArgumentException( | |
| "Property '" + EXPORT_LANGUAGES + "' must be a list of strings" | |
| ); | |
| } | |
| validatedExportLanguages.add((String) item); | |
| } | |
| props.setExportLanguages(validatedExportLanguages); | |
| } |
| props.setPseudoLocalization(PseudoLocalization.CONFIGURATOR.buildFromMap(PropertiesBuilder.getMap(map, PSEUDO_LOCALIZATION))); | ||
| PropertiesBuilder.setPropertyIfExists(props::setExportLanguages, map, EXPORT_LANGUAGES, List.class); | ||
| } |
There was a problem hiding this comment.
There’s no test exercising parsing of the new export_languages field from an actual config map/file (i.e., via the PropertiesBuilder pipeline). The added DownloadActionTest sets exportLanguages directly on the bean, so a regression in config parsing would go unnoticed. Consider adding a properties-building test that writes export_languages into a config file and asserts it ends up on PropertiesWithFiles (and/or affects the build request).
|
|
||
| ProjectClient client = mock(ProjectClient.class); | ||
| when(client.downloadFullProject(null)) | ||
| .thenReturn(ProjectBuilder.emptyProject(Long.parseLong(pb.getProjectId())) |
There was a problem hiding this comment.
Potential uncaught 'java.lang.NumberFormatException'.
|
|
||
| long buildId = 42L; | ||
| when(client.startBuildingTranslation(eq(expectedRequest))) | ||
| .thenReturn(buildProjectBuild(buildId, Long.parseLong(pb.getProjectId()), "finished", 100)); |
There was a problem hiding this comment.
Potential uncaught 'java.lang.NumberFormatException'.
No description provided.