Fix/remove default engine config #578
Conversation
|
It would be nice to see those two attribute go (including their definitions from Role::Engine). However; digging through the commit history, they were added in 82f4574, with the next commit (eb2a509) adding |
|
I'm happy you found this problem. I can now fix it. I already added what should be a fix for the travis build fail. |
|
I think I fixed it. @veryrusty, what do you think? |
|
@xsawyerx sorry it took so long to review this. Agree that the defaults for This will also help resolve other outstanding issues, like #520. |
In GH #578, Russell explains that if the File logger (Dancer2::Logger::File) is created with a declared log_dir and file_name, these attributes aren't used or needed. We save memory/CPU on this. Why not?
|
@veryrusty Updated with the change you suggested. Do you think we're good to go? We should probably write tests for it. |
Default engine configurations are configurations that are automatically sent to every engine. This makes no sense, because each engine will have its own configuration. An example would be a template engine that has a template object inside it. The template object gets sent the default configuration. The default configuration contains two keys - environment and location - which either means nothing to the template object, or it means something probably different. Instead, these should be sent as attributes to the engines. They can be configured in the Engine role and automatically fed. Thing is, I couldn't find anything that actually uses them and needs them, so I removed it completely.
Perl 5.18 doesn't like us setting $_, so we change it to a more explicit - yet not as nice - code.
…ults: Dancer2::Logger::File actually needs the attributes environment/location which we removed in this branch. It is set in the Engine role. This commit does two things: 1. Put those attributes in the File logger module 2. Give them the proper values using the App via the Context The 2nd item is the most horrible way of doing it. I'm not sure how else to do it, but it will need to be changed as part of the Context decoupling work which is something we definitely want to do.
In GH #578, Russell explains that if the File logger (Dancer2::Logger::File) is created with a declared log_dir and file_name, these attributes aren't used or needed. We save memory/CPU on this. Why not?
|
I've had another look over this (in a very sleep deprived state). Giving it a 👍, though more tests would be better 😄 |
|
Let's make sure we add some tests for it before merging. I'm gonna mark it as beginner friendly so anyone who wants to help with the tests could. |
In GH #578, Russell explains that if the File logger (Dancer2::Logger::File) is created with a declared log_dir and file_name, these attributes aren't used or needed. We save memory/CPU on this. Why not?
|
Added tests and merged! |
|
👯 @xsawyerx++ |
This is what I refer to in issue #577.
Commit message of the actual work commit: