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

Modifies WAServerManager to support subclasses to be defined as default #1344

Merged
merged 2 commits into from
Mar 4, 2023

Conversation

eMaringolo
Copy link
Contributor

VAST, and probably other dialects, have a subclass of WAServerManager that is used instead of it.

Since WAServerManager[default] is a class instance variable, then sending the message default to WAServerManager will always answer an instance of itself. Which is more troubling if you access it by means of WAAdmin defaultServerManager.

This PR modifies the default class instance variable to be a class variable (named Default), and provides a beDefault method that can be used on subclasses to be the "singleton" default.

@marschall
Copy link
Contributor

Out of curiosity, why do you need to subclass WAServerManager? Is there something we can do so that you don't have to do this?

@eMaringolo
Copy link
Contributor Author

The WASstServerManager in VAST provides logging of the start/stop events and also a callback interface (think of an "Announcer") to notify these events to the visual control panel can reflect the state without unnecessary polling.

Copy link
Member

@jbrichau jbrichau left a comment

Choose a reason for hiding this comment

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

Hi Esteban. Thanks for this and sorry for having skipped this PR several weeks ago.
I just wanted to ask a name change for the method initializeDefaultValueHolder to reflect it does not reinitialize. I suggest ensureDefaultValueHolder.

I'm all pro using class vars rather than class instvars for this. I'm not sure if this was a mistake or deliberate choice. It seems strange for a 'default' singleton implemention though, so I suspect the former.

@jbrichau jbrichau merged commit 5c41593 into SeasideSt:master Mar 4, 2023
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 this pull request may close these issues.

3 participants