fix: correct detection of obsolete files #776
Conversation
anbraten
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #776 +/- ##
============================================
+ Coverage 63.73% 63.78% +0.05%
- Complexity 1504 1505 +1
============================================
Files 220 220
Lines 6294 6297 +3
Branches 944 944
============================================
+ Hits 4011 4016 +5
Misses 1778 1778
+ Partials 505 503 -2 ☔ View full report in Codecov by Sentry. |
src/test/java/com/crowdin/cli/commands/functionality/ObsoleteSourcesUtilsTest.java
Show resolved
Hide resolved
|
Not sure where the windows issue was exactly coming from. Probably some path separator mismatch. |
|
@anbraten yes, most likely it is related to paths. We already have some similar tests, and the easiest option here is to have separate test cases for Linux/Mac and Windows operating systems. These tests have the following annotations: @DisabledOnOs({OS.LINUX, OS.MAC})
@DisabledOnOs(OS.WINDOWS)Or we can somehow normalize the paths to unify them for those environments. |
| @@ -29,7 +29,10 @@ public static Map<String, File> findObsoleteProjectFiles( | |||
|
|
|||
| private static boolean checkExportPattern(String exportPattern, File file) { | |||
| String fileExportPattern = ProjectFilesUtils.getExportPattern(file.getExportOptions()); | |||
| return exportPattern.equals(fileExportPattern != null ? Utils.normalizePath(fileExportPattern) : null); | |||
| if (fileExportPattern == null) { | |||
There was a problem hiding this comment.
@andrii-bodnar What do you think of this approach?
There was a problem hiding this comment.
Does this mean that files with the null export pattern will be considered obsolete and deleted, or the opposite?
There was a problem hiding this comment.
It basically uses all projectFiles (files received by the api) and checks for each file:
- if that file matches the config patterns (source-pattern, ignore and preserveHierarchy)
- if it matches the exportPattern (if the api returns null for the export pattern we ignore it now by this change)
- and if the file has
isFileDontHaveUpdate=true(which seems to check if the file is not part of the local files anymore)
There was a problem hiding this comment.
I adjusted some var and method names so they match the rest of the code and are hopefully more descriptive.
There was a problem hiding this comment.
@anbraten great, I just tested it locally and the fix works for me, thank you! 🚀
closes #775
Repro: https://github.com/anbraten/repro-crowdin-delete-obsolete
I am not sure why the
exportPatternof a source-file was checked to see that it is obsolete. It works for me without the check, but I would also be happy to fix the check if it's needed and someone could explain me how it should work.