Refactor: Align cache path with Laravel conventions #7
Conversation
sajjadhossainshohag
changed the title
sajjadhossainshohag
changed the title
|
Thx for the PR! I'm going to look at it this week |
QuentinGab
left a comment
QuentinGab
left a comment
There was a problem hiding this comment.
Replacing base_path with storage_path is ok.
But I'm not gonna merge the progress part cause it will just make it less performant
| { | ||
| Translator::clearCache(); | ||
|
|
||
| info('Cache cleared'); |
There was a problem hiding this comment.
Why note prompt info function?
There was a problem hiding this comment.
Before:
Now:
For more highlighting the message.
src/Commands/MissingCommand.php
Outdated
| private function syncMissingKeysWithProgress($translator, string $locale, array $missing): void | ||
| { | ||
| $progress = progress(label: 'Syncing...', steps: count($missing)); | ||
| $progress->start(); | ||
|
|
||
| foreach ($missing as $key => $value) { | ||
| $translator->setTranslation($locale, $key, null); | ||
| $progress->advance(); | ||
| } | ||
|
|
||
| $progress->finish(); | ||
| $this->components->info(count($missing) . ' missing keys have been synced.'); |
There was a problem hiding this comment.
Using progress is not really relevant here because setTranslations will add all missing translations in a single write. Doing a for-loop will just make it less performant.
There was a problem hiding this comment.
I know that, but installing the package in my big project took a lot of time. So, that's why I added a progress bar for knowing about progress.
We can show progress bar with the option, like --without-progress or with-progress. Please let me know your opinion.
There was a problem hiding this comment.
Understood, but the part taking time is getMissingTranslations not setTranslations.
I should certainly add a progress callback to getMissingTranslations so it could be displayed it the terminal. I'm gonna look at it this week, thx for the feedback!
QuentinGab
left a comment
QuentinGab
left a comment
There was a problem hiding this comment.
Until I implement a progress callback, I'm gonna keep MissingCommand like it is.
Could you remove your change on the file?
src/Commands/MissingCommand.php
Outdated
| private function syncMissingKeysWithProgress($translator, string $locale, array $missing): void | ||
| { | ||
| $progress = progress(label: 'Syncing...', steps: count($missing)); | ||
| $progress->start(); | ||
|
|
||
| foreach ($missing as $key => $value) { | ||
| $translator->setTranslation($locale, $key, null); | ||
| $progress->advance(); | ||
| } | ||
|
|
||
| $progress->finish(); | ||
| $this->components->info(count($missing) . ' missing keys have been synced.'); |
There was a problem hiding this comment.
Understood, but the part taking time is getMissingTranslations not setTranslations.
I should certainly add a progress callback to getMissingTranslations so it could be displayed it the terminal. I'm gonna look at it this week, thx for the feedback!
|
@QuentinGab Removed my changes, |
sajjadhossainshohag
changed the title
|
I took the time to add the progress indicator: https://github.com/ElegantEngineeringTech/laravel-translator/releases/tag/v2.1.1 |
|
@QuentinGab Thanks man. |
2 participants
Hey @QuentinGab,
I made a few things. Review this and if any changes are needed, you can change it.
storagefolder for better alignment with Laravel conventions.