This repository has been archived by the owner. It is now read-only.

Move config methods to JApplicationBase #1732

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@mbabker
Member

mbabker commented Dec 3, 2012

To clean up a little bit of duplicated code, this moves the get, set, loadConfiguration, and fetchConfigurationData methods from the child classes of JApplicationBase into the parent class itself.

@eddieajau

This comment has been minimized.

Show comment
Hide comment
@eddieajau

eddieajau Dec 3, 2012

Contributor

I think most of it is ok; it's perfectly reasonable to assume an application is going to need configuration out of the gate. The only problem I have is what to do with fetchConfigurationData. I don't think that should be in Base, and looking at it, I'm not sure it technically belongs in Web and Cli.

Contributor

eddieajau commented Dec 3, 2012

I think most of it is ok; it's perfectly reasonable to assume an application is going to need configuration out of the gate. The only problem I have is what to do with fetchConfigurationData. I don't think that should be in Base, and looking at it, I'm not sure it technically belongs in Web and Cli.

@dongilbert

This comment has been minimized.

Show comment
Hide comment
@dongilbert

dongilbert Dec 3, 2012

Contributor

What are the arguments against having it in the base class? Most (?) apps would require some sort of configuration data.

Contributor

dongilbert commented Dec 3, 2012

What are the arguments against having it in the base class? Most (?) apps would require some sort of configuration data.

@eddieajau

This comment has been minimized.

Show comment
Hide comment
@eddieajau

eddieajau Dec 3, 2012

Contributor

The loadConfiguration method is fine. I'd be happier if fetchConfigurationData in Base was abstract.

Contributor

eddieajau commented Dec 3, 2012

The loadConfiguration method is fine. I'd be happier if fetchConfigurationData in Base was abstract.

@LouisLandry

This comment has been minimized.

Show comment
Hide comment
@LouisLandry

LouisLandry Dec 4, 2012

Contributor

Yep, I agree with @eddieajau on that. I am fine assuming that all apps will have configuration, but I think I'd rather have the fetchConfigurationData method be abstract. Continuing that it would make sense that it remain abstract in JApplicationWeb, JApplicationCli and JApplicationDaemon.

Contributor

LouisLandry commented Dec 4, 2012

Yep, I agree with @eddieajau on that. I am fine assuming that all apps will have configuration, but I think I'd rather have the fetchConfigurationData method be abstract. Continuing that it would make sense that it remain abstract in JApplicationWeb, JApplicationCli and JApplicationDaemon.

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Dec 4, 2012

Member

Question - Were JApplicationWeb or JApplicationCli intended to be abstract, or is it reasonable to assume they could theoretically work on their own (as the code seems to have indicated for the last year)? Abstracting the method down to those classes would mean abstracting classes that aren't already, an interesting change in the API.

Member

mbabker commented Dec 4, 2012

Question - Were JApplicationWeb or JApplicationCli intended to be abstract, or is it reasonable to assume they could theoretically work on their own (as the code seems to have indicated for the last year)? Abstracting the method down to those classes would mean abstracting classes that aren't already, an interesting change in the API.

@LouisLandry

This comment has been minimized.

Show comment
Hide comment
@LouisLandry

LouisLandry Dec 4, 2012

Contributor

Ya I thought about that. Realistically you can't build anything useful without extending them, so I'm fine making them abstract.

Contributor

LouisLandry commented Dec 4, 2012

Ya I thought about that. Realistically you can't build anything useful without extending them, so I'm fine making them abstract.

@mbabker

This comment has been minimized.

Show comment
Hide comment
@mbabker

mbabker Dec 4, 2012

Member

I'll do some work then to tweak this out and submit a new PR when that's ready.

Member

mbabker commented Dec 4, 2012

I'll do some work then to tweak this out and submit a new PR when that's ready.

@mbabker mbabker closed this Dec 4, 2012

@mbabker mbabker deleted the mbabker:config branch Feb 23, 2013

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