bamaer commented on PR #9246: URL: https://github.com/apache/hop/pull/9246#issuecomment-6625087190
Thanks @leehaut for picking this up. The success-condition and double-count fixes look right, and the tests cover cases 1–6 of #9175. A few things before this can be merged: ## Must fix **1. Move files: a successful "Move to folder" now counts as an error when the source is a folder.** In `moveFile()`, the `move_file` case where the move-to target doesn't exist yet moves the file but never sets `retVal = true` or calls `updateSuccess()`. Before this PR that didn't matter for folder sources, because `moveOneFile()` always ended with `entryStatus = true`. `moveOneFile()` now passes on the result of `moveFile()`, so each file moved this way adds one error. - Scenario: source is a folder, the destination file exists, *If destination file exists* = *Move to folder*, and the move-to folder has no file with that name. - Before: true. After: false, with one error per file. - With a single file as the source this was already wrong on main, and the same change fixes it. - Suggested fix: add `updateSuccess(); retVal = true;` at the end of that branch, plus a test for this case (the current tests only cover `FAIL`). **2. Zip file docs vs code: *Get arguments from previous* with no result rows.** The new NOTE says the action is false in this case. The code only fails when `!rows.isEmpty()`. With an empty list, `previousResult` is passed on unchanged, so the action inherits the previous result. Either fail when `anyZipped` is false, or keep the old wording for this case. **3. Move files docs: the NOTE still says "a file that already exists counts as two errors".** This PR fixes the double count, so that sentence can go. **4. Delete files docs: a result row with fewer than two fields.** The doc says "the remaining rows are not processed". `populateDataForWorkflowExecution()` now throws before any row is processed, so nothing is deleted, not even for the rows before the short one. Failing before deleting anything is reasonable, but the doc should say that nothing is deleted and the action is false. ## Minor - **Delete files:** the inner `try`/`catch (PatternSyntaxException)` around `compileWildcard()` is not needed. The pattern is now compiled outside the file selector, so the existing `catch (Exception)` in `processFile()` logs the same `CouldNotProcess` message and returns false. - **Zip file, several rows:** rows with an empty source or zip file name are skipped. If at least one other row is zipped the action is true, and if every row is skipped it is false. That is fine, but the "Result rows" text should say so. - **Move files:** the `// caller counts the error once` comment sits on its own line before the outer `case FAIL ->`, with odd indentation. A trailing comment, as on the inner `case FAIL`, reads better. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
