Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Refactor SpringApplication#setupProfiles and #addPropertySources methods #467

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Contributor

cbeams commented Mar 11, 2014

Here's one to consider prior to GA, especially given that it would be a breaking API change.

Regarding SpringApplication#setupProfiles:

  • configureX vs. setupX is more aligned with existing naming conventions throughout the core framework.
  • "setup" is not a verb.
  • Configuring profiles is just one of the things a user could do with the Environment they're being handed (manipulating property sources would be another). Thus, configureEnvironment is a more accurate name.

As I was writing the above, I just noticed that there is also an #addPropertySources method that accepts the Environment as well as the String[] of args passed to the #run method.

How about refactoring this arrangement as follows?

  • Rename #addPropertySources(Environment, String[]) to #configurePropertySources(Environment, String[]) ("configure" is better here as well, given that property sources could be removed or reordered, not just "added").
  • Rename #setupProfiles(Environment) to #configureProfiles(Environment, String[]) (accept the String[] of args here too)
  • Add #configureEnvironment(Environment, String[]) as a template method that delegates to the above two methods in that order.
  • Update SpringApplication#run to call the new #configureEnvironment method instead of calling the former methods individually.
  • Add JavaDoc @see references to each method to inform folks of the relationship between them.

I bring all of this up because we're now hooking into #setupProfiles in the Sagan application to codify and enforce our profile semantics, and we will be pointing users to it in blog posts and documentation. I was really glad to find this method—it suits our needs perfectly, and I could see using it becoming a best practice for apps that have anything beyond the most trivial profile arrangements.

Owner

dsyer commented Mar 10, 2014

Sounds reasonable, as long it preserves the existing behaviour (especially with XD), but I think it does.

I would prefer that users (and Sagan "best practices") used listeners and initializers, and also that they used SpringApplicationBuilder, rather than resorting to subclassing SpringApplication though. Can you do it that way?

PR still welcome for the re-org in any case, if you think it matters.

Contributor

cbeams commented Mar 11, 2014

I would prefer that users (and Sagan "best practices") used listeners and initializers, and also that they used SpringApplicationBuilder, rather than resorting to subclassing SpringApplication though. Can you do it that way?

Not really. Take a look at SaganApplication. I think that IS-A semantics here are actually appropriate. There are two "sagan applications"—indexer and site—and they both have the same profile rules to be enforced. Doing this through subclassing and overriding #configureProfiles is a nice, centralized approach and avoids the need to repeat that logic wherever SpringApplicationBuilder otherwise would have been used.

I don't disagree that SpringApplicationBuilder is probably often the right choice, but since we have more than one app involved here, I think the approach is merited. Certainly open to further discussion though, if on review you or others think there are still better ways to get this done.

PR still welcome for the re-org in any case, if you think it matters.

Submitted here, thanks.

@cbeams cbeams closed this in e10856f Mar 11, 2014

@dsyer dsyer added this to the 1.0.0.RELEASE milestone Mar 11, 2014

cbeams added a commit to spring-io/sagan that referenced this pull request Mar 20, 2014

@cbeams cbeams referenced this pull request in spring-io/sagan Mar 20, 2014

Merged

Upgrade to Spring Boot 1.0.0.RC5 #309

cbeams added a commit to spring-io/sagan that referenced this pull request Mar 20, 2014

gigfork pushed a commit to boostrack/spring-boot that referenced this pull request Apr 21, 2014

mdeinum added a commit to mdeinum/spring-boot that referenced this pull request Jun 6, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment