Skip to content
This repository has been archived by the owner on Mar 15, 2022. It is now read-only.

[JBEAP-5040] No simple way to refresh the Runtime -> Subsystems -> JN… #263

Merged
merged 2 commits into from Jul 14, 2016

Conversation

xstefank
Copy link
Contributor

/**
* @author Martin Stefanko
*/
public interface JndiViewManagement {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate what's the reasoning behind this interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no specific need for it. I made it to group possible further extensions or features to be added to the JndiPresenter class. In my opinion, it is no direct association to the funcionality the class should represent. There is very similar interface TXMetricManagement.

Thank you for the review. If you wish, there is no problem to remove it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not necessary to have this interface now. IMO interfaces make sense when there are more than one implementation. I see your point regarding TXMetricManagement, but TBH I think this interface could be removed as well.

A generic RefreshMetrics interface, which is implemented by all metric presenters, would make sense to me. But that would be a bigger refactoring.

To sum it up: Could you please remove TXMetricManagement

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, do you want me to delete both interfaces in one commit ? Do I understand correctly ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the confusion. There was a typo in my last comment. I meant only to remove the JndiViewManagement interface.

@hpehl hpehl merged commit b8f6b9c into hal:master Jul 14, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants