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

feat: Ability to change reload strategy from AutoMapper::create() #183

Merged
merged 1 commit into from
Sep 16, 2024

Conversation

MrMeshok
Copy link
Contributor

@MrMeshok MrMeshok commented Sep 4, 2024

No description provided.

@joelwurtz
Copy link
Member

joelwurtz commented Sep 4, 2024

Thanks for this PR, Could this use the Configuration class instead of being a parameter, i think we should go this way for every configuration parameter, it will be easier for BC break rather than having a new parameter

However you may need to change the symfony bundle in order to make it work (not sure)

@MrMeshok
Copy link
Contributor Author

MrMeshok commented Sep 4, 2024

Yea, I thought about putting it in Configuration, but create method already has cacheDirectory argument, so it seems natural to put reloadStrategy there too. Also, I did have a look in the symfony bundle, as far as I can tell, AutoMapper creates from DI without using create method, and reloadStrategy from FileLoader gets replaced through container

Copy link
Member

@Korbeil Korbeil left a comment

Choose a reason for hiding this comment

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

Thanks for this feature @MrMeshok, could you move the parameter to the Configuration class and add a note about your change in the CHANGELOG please ? 🙏

@MrMeshok
Copy link
Contributor Author

@Korbeil Done. I also did a small change in Symfony bundle, although it is not necessary

@Korbeil Korbeil merged commit 9de87a7 into jolicode:main Sep 16, 2024
5 checks passed
@Korbeil
Copy link
Member

Korbeil commented Sep 16, 2024

Thanks for your contribution @MrMeshok !

@MrMeshok MrMeshok deleted the reload-strategy-from-create branch September 16, 2024 10:34
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.

3 participants