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

Support user override lang folders #40

Closed
wants to merge 4 commits into from

Conversation

asika32764
Copy link
Contributor

@asika32764 asika32764 commented Jul 25, 2023

CronTranslator::parse('@weekly', 'pt')
                ->addLangDir(__DIR__ . '/my/custom/lang')
                ->translate();

// OR

CronTranslator::parse('@weekly')
                ->addLangDir(__DIR__ . '/my/custom/lang')
                ->translate('pt', true);

To keep B/C, I expose CronExpression class that user can configure it. I decided not to add 4th params to translate() method.

CronTranslator class added 2 new public methods

$expr = CronTranslator::parse($cron, $lang, $24hour); // To create a new CronExpression

// Configure it

CronTranslator::translateExpression($expr); // Return translated string

// Old way still works
CronTranslator::translate('cron', 'en', true); // Works

@lorisleiva
Copy link
Owner

Hi there, thanks for this. I like the idea of this PR but I'm not a big fan of pushing all the logic from the CronTranslator to the CronExpression. It feels like these directories should probably be stored on the CronTranslator instead but that may require a bit of refactoring first.

@asika32764
Copy link
Contributor Author

Yes, I see what you want.

I push all logic to CronExpression just because I want to keep B/C and avoid too large rewrite of whole project.

If you have new idea about how to implement this, fell free to take my code to rewrite it.

Just depends on your disicion.

@lorisleiva
Copy link
Owner

I'm closing this because PR #42 adds a new LanguageLoader class which could be used to support language folder overrides if you'd like to give it a go. 🙂

@lorisleiva lorisleiva closed this Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants