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

Add "Application" Habitat class for app settings #561

Closed
stephendolan opened this issue Sep 16, 2020 · 5 comments · Fixed by #694
Closed

Add "Application" Habitat class for app settings #561

stephendolan opened this issue Sep 16, 2020 · 5 comments · Fixed by #694

Comments

@stephendolan
Copy link
Member

I found that I could clean up a lot of code if I were able to access the application name through a setting like Application.settings.name, so I created the following Habitat class and settings in config/application.cr:

class Application
  Habitat.create do
    setting name : String
    setting tagline : String
    setting support_email : String
  end
end

Application.configure do |settings|
  settings.name = "Emerson Executives"
  settings.tagline = "The purpose of life is not to be happy. It is to be useful, to be honorable, to be compassionate, to have it make some difference that you have lived and lived well."
  settings.support_email = "no-reply@emersonexecutives.com"
en

These are really useful to have for things like email content where you may want to use the application name ("Welcome to Emerson Executives!") in the subject.

It was pretty easy to add on my own, but even after using Lucky for a while it wasn't obvious where something like this should go. Even if we had content like this and everything was commented out, it could be useful in leading users in the right direction and providing a clearer path for reducing the use of magic numbers and strings in your code.

@paulcsmith
Copy link
Member

I'm thinking that this might be better as something people add later rather than being built-in. Some of this stuff might be better as a constant, or as a regular method. For example

class Application
  def self.name
      "Emerson Executives"
  end
end

Since the name won't be configured at runtime (I'd guess) this might work better. I'm also worried this will become a sort of "junk drawer" of config. For example, the support email address might be clerer in configs/emails.cr and you add configurtion to your SupportEmail:

abstract class SupportEmail < BaseEmail
  Habitat.create do
    setting from_address : String = "defaultsupport@foo.com"
  end
end

So my thoughts are this should be closed and maybe we document some strategies in the docs instead. Thoughts?

@stephendolan
Copy link
Member Author

I definitely feel you on the "Junk Drawer" point, @paulcsmith.

While specific locations might make more sense functionally for experienced Lucky users, to be honest I was thinking of this more in the vein of a new Lucky user who may not be familiar with all the knobs and buttons they can use in a new Lucky app, or with the ability/comfort to extend or add new classes like SupportEmail. It'd be really easy to say "Just go to this file and add your custom app stuff to change from the default app name, email, etc".

The same advantage would exist as Lucky gains adoption and more and more folks have starting points or template applications. Having one place to go to change out all of the template variables for libraries and whatnot would be pretty nifty for folks getting started.

I still could do a LuckyCast on the idea of adding custom configuration and include it in templates I create manually, so not incorporating this into the default app certainly wouldn't be the end of the world. Ultimately you may be right that the framework itself shouldn't provide this functionality, but as a previous Rails user I will say that the (admittedly junk drawer-y) config/application.rb file felt pretty handy/intuitive.

That's a long way of saying that my feelings won't be hurt if we close the issue, and I'll yield to your judgment on this one in terms of what Lucky should provide "out of the box".

@paulcsmith
Copy link
Member

This is a very good point. You've convinced me.

I think we can go forward with this but I think we need to guide people to others way of config as well. So in the docs we could start by using the Application config, but also have some more examples below it for when things start getting out of hand, or you need something specific to a class.

I think we should also link to that in the generated config/application.cr

Does that sound like a good approach to you @stephendolan?

@stephendolan
Copy link
Member Author

Sounds like a great idea, Paul!

I'll open an issue in the CLI repo to add this to the default template, and will ensure that the file I generate is pretty heavily-commented with some common "Do"s and "Don't"s.

I'll add you as a reviewer there specifically (no rush on reviewing) so that you're aware of it.

@paulcsmith
Copy link
Member

Sounds good! And feel free to ping me in Discord if it takes too long. Sometimes I miss stuff here :P

Could you also open an issue for documenting this in the config guide?

@stephendolan stephendolan transferred this issue from luckyframework/lucky Oct 21, 2020
jwoertink added a commit that referenced this issue Oct 28, 2021
* Adding in blank Application level config file. Fixes #561

* formatting

* a little wording and clarity
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 a pull request may close this issue.

2 participants