snu-quiqcl / qiwis

QuIqcl Widget Integration Software
MIT License
5 stars 2 forks source link

Use global constant for channel names #186

Closed kangz12345 closed 10 months ago

kangz12345 commented 1 year ago

In the example codes, the channel names are hard-coded in the app codes, and it should be consistent to the ones in the "setup.json".

We can mitigate this problem by defining constants for the channel names in each app code to prevent using the string-literal channel names everywhere (in which case it is hard to rename the channels).

However, we still have to define such constants in each app code consistently. For example, assume that every app uses the logging channel. Then they have to define, e.g., LOG_CHANNEL="log" in each app code.

If we use the global constant which is suggested in #185, we can tackle this issue further. However, I am not sure yet how effective this would be. For example, even if we use the global constant namespace, we still have to use the agreed constant name and any mismatch cannot be detected statically (e.g., by IDEs).

Any opinions on this?

BECATRUE commented 1 year ago

I also think that setting channel names as global constants is the best solution!

BECATRUE commented 10 months ago

@kangz12345 I realized a problem while I tested several global constants. How can I access them in each app?

In an app class, neither self._constants nor BaseApp._constants can access them well.

I think we set the constants not in BaseApp, but in each app instance directly.

kangz12345 commented 10 months ago

Referring to #189:

For the access, BaseApp now provides a read-only property constants.

So self.constants should work. If you still have problems, please let me know.

BECATRUE commented 10 months ago

It doesn't work well. Specifically, the constants is an empty namespace.

kangz12345 commented 10 months ago

Could you show me the config.json file you used? It must have "constant" section.

BECATRUE commented 10 months ago

The config.json for test is as follows:

{
    "app": {
        "numgen": {
            "module": "examples.numgen",
            "cls": "NumGenApp",
            "show": true,
            "pos": "left",
            "channel": ["db"],
            "args": {
                "table": "number"
            }
        },
        "dbmgr": {
            "module": "examples.dbmgr",
            "cls": "DBMgrApp",
            "show": true,
            "pos": "right",
            "channel": []
        },
        "poller": {
            "module": "examples.poller",
            "cls": "PollerApp",
            "show": true,
            "pos": "bottom",
            "channel": ["db"],
            "args": {
                "table": "B"
            }
        },
        "logger": {
            "module": "examples.logger",
            "cls": "LoggerApp",
            "show": true,
            "pos": "bottom",
            "channel": []
        }
    },
    "constant": {
        "icon_path": "resources/icon.jpg",
        "background_path": "resources/background.jpg"
    }
}

I printed the following two properties.

BECATRUE commented 10 months ago

I made a simple example.

test1.py

from test2 import B

class A(B):
    def show(self):
        print(self.cnt)

test2.py

class B:
    _cnt = 0

    @property
    def cnt(self):
        return B._cnt

def main():
    B._cnt = 1
    from test1 import A
    ins = A()
    ins.show()

if __name__ == "__main__":
    main()

Result

$ python test2.py
0

Isn't this the same case as that we implemented?

kangz12345 commented 10 months ago

Hmm okay interesting 😄 I think the two B in test1.py and test2.py are distinct class objects, and therefore each of them has its own namespace. In other words, test1.B._cnt and test2.B._cnt seem to be different variables. I will make a bug issue about this. Thank you for reporting!

BECATRUE commented 10 months ago

After fixing the bug in #215 and implementing this issue, I plan to release the next version. Is it okay? @kangz12345

kangz12345 commented 10 months ago

Oh.. actually this makes a circular import, which should be avoided. This might be a big problem than we thought.

kangz12345 commented 10 months ago

Please continue discussion in #215 since it is out of scope of this issue.

kangz12345 commented 10 months ago

Instead of implement it in this way, @BECATRUE and I decided to use apps' constructor arguments instead of the global constant namespace. Constructor gives more straightforward intuition about what channel names users should provide.