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

fix: correct detection of obsolete files with double asterisk pattern #794

Merged
merged 12 commits into from
May 16, 2024

Conversation

anbraten
Copy link
Contributor

closes #790

@anbraten anbraten marked this pull request as ready for review May 15, 2024 09:01
@anbraten anbraten changed the title fix: correct detection of obsolete files 2 fix: correct detection of obsolete files with double asterisk pattern May 15, 2024
@anbraten
Copy link
Contributor Author

@andrii-bodnar @finebel This should be it.

@andrii-bodnar
Copy link
Member

@anbraten thanks a lot for your contribution! 🚀

The build has failed, could you please take a look?

Copy link

codecov bot commented May 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.80%. Comparing base (94fb3bf) to head (fb58bb6).
Report is 1 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #794      +/-   ##
============================================
+ Coverage     63.78%   63.80%   +0.03%     
- Complexity     1505     1506       +1     
============================================
  Files           220      220              
  Lines          6297     6298       +1     
  Branches        944      944              
============================================
+ Hits           4016     4018       +2     
  Misses         1778     1778              
+ Partials        503      502       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andrii-bodnar
Copy link
Member

@finebel could you please download the build and check whether it fixes the issue for you?

https://github.com/crowdin/crowdin-cli/actions/runs/9093764457 (artifacts)

You can run it as follows:

java -jar crowdin-cli-x.x.x.jar push

@finebel
Copy link

finebel commented May 15, 2024

@anbraten Thanks for the fast implementation. With https://github.com/crowdin/crowdin-cli/actions/runs/9093764457 I can't reproduce the issue described in #790 👍.

However, I encountered the following behaviour. I'm not sure whether it's a bug or intended. It might also be unrelated to the new changes (I haven't tested it with the current release version).
EDIT: I have tested it with the current 3.19.3 release and the behaviour is the same. So it seems to be unrelated.

1. crowdin.yml

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

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

With the following project structure:

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

2. Upload

java -jar crowdin-cli-3.19.3.jar upload sources \
    --auto-update \
    --delete-obsolete \
    --identity crowdin_credentials.yml

Result:
Screenshot 2024-05-15 at 14 54 14

3. Change crowdin.yml

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

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

(added /**/someDir/ to source and translation pattern).

Updated local project structure:

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

4. Upload again

java -jar crowdin-cli-3.19.3.jar upload sources \
    --auto-update \
    --delete-obsolete \
    --identity crowdin_credentials.yml

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

Result:
Screenshot 2024-05-15 at 14 58 26

@andrii-bodnar What do you think?

@andrii-bodnar
Copy link
Member

@finebel thanks for confirming that the original issue is now fixed 🚀

I think the second issue is somehow related to the double ** in the source and translation pattern. The current implementation probably doesn't handle such cases well. Honestly, it's quite tricky to handle such complex patterns 🙂

@anbraten
Copy link
Contributor Author

anbraten commented May 15, 2024

Lemme do some more testing and check if I can fix that issue directly as well.

@anbraten
Copy link
Contributor Author

anbraten commented May 15, 2024

That issue seems to be coming from a completely different part now. The exportPattern is wrong:

  • Expected: "/someDir/someFeature/translations/%osx_locale%/%original_file_name%"
  • Got: "/someDir/someFeature/someDir/someDir/someFeature/translations/%osx_locale%/%original_file_name%"

I get this exportPattern directly after fetching the project from the api client.

@andrii-bodnar
Copy link
Member

Hmm, so I guess it happens at the moment the source files are uploaded to Crowdin when the CLI gets the patterns, transforms ** to the actual path and then passes the exportPattern in the request. Then that pattern is set as the export pattern for the source file

@finebel
Copy link

finebel commented May 15, 2024

Just as a side note: It's much more important to me that the issue in #790 is fixed now. At the moment, I don't have a use case which would require the second issue to be solved (I encountered it mainly out of curiosity). Of course, it would be nice to see the second issue solved as well - but I guess it would be sufficient to fix it as part of a new PR if it's not trivial.

@andrii-bodnar
Copy link
Member

Definitely, I am going to release a new version with the fix tomorrow.

Copy link
Member

@andrii-bodnar andrii-bodnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anbraten @finebel Thank you guys for your contributions and participation! 🙌

@andrii-bodnar andrii-bodnar merged commit 2359e90 into crowdin:main May 16, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--delete-obsolete not working correctly when including /**/ in source pattern
3 participants