Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

--delete-obsolete not working correctly when including /**/ in source pattern #790

Closed
finebel opened this issue May 14, 2024 · 7 comments · Fixed by #794
Closed

--delete-obsolete not working correctly when including /**/ in source pattern #790

finebel opened this issue May 14, 2024 · 7 comments · Fixed by #794

Comments

@finebel
Copy link

finebel commented May 14, 2024

When including /**/ in the source pattern (crowdin.yml) deleted files aren't detected anymore.

Setup

crowdin.yml file:

"base_path": "."
"preserve_hierarchy": true

"files": [
  {
    "source": "/**/sources/*.strings",
    "translation": "/**/translations/%osx_locale%/%original_file_name%"
  }
]

Project structure:

├── crowdin.yml
├── crowdin_credentials.yml
└── someFeature
    └── sources
        ├── file1.strings
        ├── file2.strings
        └── file3.strings

(I don't added any translations, since that's not relevant for the issue)

To Reproduce

1. Upload sources

crowdin upload sources \
    --auto-update \
    --delete-obsolete \
    --identity crowdin_credentials.yml

2. Delete a file

rm someFeature/sources/file1.strings

3. Upload sources again

crowdin upload sources \
    --auto-update \
    --delete-obsolete \
    --identity crowdin_credentials.yml

// output
✔️  Fetching project info     
✔️  File 'someFeature/sources/file3.strings'
✔️  File 'someFeature/sources/file2.strings'
✔️  No obsolete files were found
✔️  No obsolete directories found

In Crowdin:
Screenshot 2024-05-14 at 09 16 32

Locally:

├── crowdin.yml
├── crowdin_credentials.yml
└── someFeature
    └── sources
        ├── file2.strings
        └── file3.strings

Expected

The locally deleted file1.strings is removed from Crowdin and the CLI output reflects the removal of file1.strings.

Additional information

I couldn't reproduce the issue without the /**/ pattern. Since #775 also used the /**/ pattern, I'm wondering whether there is some other difference I haven't seen so far. In https://github.com/anbraten/repro-crowdin-delete-obsolete/blob/main/crowdin.yml#L6 @anbraten doesn't use /**/ (only in the issue description /**/ is used).
When using branches, the issue appears only for existing branches. Meaning that if I run

crowdin upload sources \
    --auto-update \
    --delete-obsolete \
    --branch someBranch \
    --identity crowdin_credentials.yml

// output
✔️  Fetching project info     
✔️  Branch 'someBranch'
✔️  Directory 'someBranch/someFeature'
✔️  Directory 'someBranch/someFeature/sources'
✔️  File 'someBranch/someFeature/sources/file2.strings'
✔️  File 'someBranch/someFeature/sources/file3.strings'
✔️  No obsolete files were found
✔️  No obsolete directories found

after step 3 from above, the file1.strings is successfully deleted for someBranch from Crowdin.
Screenshot 2024-05-14 at 09 25 31

However, if I delete another file (rm someFeature/sources/file2.strings) and running

crowdin upload sources \
    --auto-update \
    --delete-obsolete \
    --branch someBranch \
    --identity crowdin_credentials.yml

// output
✔️  Fetching project info     
⏭  Branch 'someBranch' already exists in the project
✔️  File 'someBranch/someFeature/sources/file3.strings'
✔️  No obsolete files were found
✔️  No obsolete directories found

again, file2.strings is still present in someBranch on Crowdin.

Screenshot 2024-05-14 at 09 27 53

Environment

  • macOS
  • Crowdin CLI 3.19.3
@andrii-bodnar
Copy link
Member

Thanks for reporting this!

@anbraten what do you think, could it be because of the export pattern comparison?

@anbraten
Copy link
Contributor

anbraten commented May 14, 2024

I did a quick test using /test/en/**/*.md as source pattern which worked in the unit test, so I guess there is a different error related to the export pattern being set somewhere else.

@anbraten
Copy link
Contributor

Created a repro branch for this issue:

https://github.com/anbraten/repro-crowdin-delete-obsolete/tree/test2

With that test I get a setting for fileExportPattern and therefore the check (which wasn't touched in #776 AFAIK) is executed:

image

@finebel Are you sure this worked with a version previously to 3.19.3?

@finebel
Copy link
Author

finebel commented May 14, 2024

Are you sure this worked with a version previously to 3.19.3?

@anbraten No, I started using the Crowdin CLI tools with version 3.19.0 and it didn't work from the beginning (see https://community.crowdin.com/t/cli-delete-obsolete-has-no-effect-when-uploading-source-files/7455 for reference).

I have thought that the issue have been resolved with #776 since it reads very similar. But apparently it's a different bug.

@anbraten
Copy link
Contributor

Thanks. Good to know I haven't introduced that bug and my assumption it was there before was right.

@andrii-bodnar Which function could we use to check if a pattern matches another path, so we can check if:

  • exportPattern: "/**/translations/%osx_locale%/%original_file_name%"
  • and fileExportPattern: "/someFeature/translations/%osx_locale%/%original_file_name%"

match?

@andrii-bodnar
Copy link
Member

@anbraten checked the isProjectFilePathSatisfiesPatterns method and it seems to do what we need here.

What happens in the current case is that the ** is replaced with the actual path and then set to the export pattern. As a result, the translation pattern in the configuration path does not match the export pattern of the file.

Maybe we can also use the replaceDoubleAsterisk method.

@anbraten
Copy link
Contributor

Started to give the isProjectFilePathSatisfiesPatterns #794 a try. Just need to add some tests now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants