search

cloudfoundry-attic / eclipse-integration-cloudfoundry

Cloud Foundry Integration for Eclipse
Apache License 2.0
41 stars 47 forks source link

Using service name from extension instead of compiled constant #17

Closed Phanatic closed 10 years ago

Phanatic commented 10 years ago

Fixes #16. No new extension point is required, I was able to read the service name from the CloudFoundryBrandingExtensionPoint type and use NLS.bind to bind the service name to the messages.

nierajsingh commented 10 years ago

Thanks for submitting the PR. This is a far better solution that having an extension point. One thing I do want to mention before I accept the PR is that the console messages may change in the future. Although these are user visible text, it's part of internal core components of CF Eclipse so we may change them later. However, I think for the time being your changes are fine. Thanks again.

Phanatic commented 10 years ago

@nierajsingh, thanks for the insight into future changes in the messages. I'll keep an eye out for changes in this area.

Phanatic commented 10 years ago

@nierajsingh, can this be merged in?

elsony commented 10 years ago

It is a good idea to clean those messages up. A couple of thoughts:

  1. My understanding is the code controls the console message output that is specific to a particular instance of the server. Given that console is specific to a particular server already, do we still need to mention the server name or server type name? e.g. change "Pushing application to Cloud Foundry server" to "Pushing application to the server" or "Pushing application "? Given that the message is for a given server already, the app name may be more useful to the user on those messages. If we have enough information in previous console messages for identifying the app name, may be even "Pushing application" is good enough.
  2. If we want to keep the server name substitution, the current proposal is to use the service name on the branding extension. Should we use the getServerDisplayName(String) instead since that is the display name of the branding? The alternative is to use the getServer().getName() which is the actual instance name that the user sees on the Servers view.

Any thoughts?

nierajsingh commented 10 years ago

I am leaning towards just making general statements.

"Pushing application" sounds fine as the application name is already mentioned when the console is started. The server name is already mentioned in the console view title, so I think we can skip mentioning the server name.

If we are all in agreement, maybe this PR can be modified to just remove "Cloud Foundry" from those messages for now and make the statements more general.

Thanks.

elsony commented 10 years ago

"Publishing application" sounds good to me given that the server and app name has already been mentioned.

Phanatic commented 10 years ago

Yep, I can make the messages more general across the board in the console and push a change later today.

Thanks, Phani

On Jul 24, 2014, at 9:25 AM, Elson Yuen notifications@github.com wrote:

"Publishing application" sounds good to me given that the server and app name has already been mentioned.

— Reply to this email directly or view it on GitHub.