Skip to content

[ENH] Pull YAML feed of notifications on startup, refactor notifications#3933

Merged
lanzagar merged 3 commits intobiolab:masterfrom
irgolic:yaml-notifications
Aug 30, 2019
Merged

[ENH] Pull YAML feed of notifications on startup, refactor notifications#3933
lanzagar merged 3 commits intobiolab:masterfrom
irgolic:yaml-notifications

Conversation

@irgolic
Copy link
Copy Markdown
Member

@irgolic irgolic commented Jul 11, 2019

Should be merged after biolab/orange-canvas-core#15.

Issue

Likely the final pull request implementing #3372.

Description of changes
  • Download and parse list of notifications in YAML format.
  • Refactored notifications so as to expose a simpler interface to the programmer.
  • Fix preferences access while parsing YAML (QSettings dictionary interface)
Includes
  • Code changes
  • Tests
  • Documentation

@irgolic irgolic requested review from markotoplak and robertcv July 11, 2019 14:15
@irgolic irgolic force-pushed the yaml-notifications branch from e53e676 to 779d16a Compare July 17, 2019 09:18
@irgolic irgolic removed the request for review from robertcv July 17, 2019 11:00
@irgolic irgolic force-pushed the yaml-notifications branch 2 times, most recently from e7c1bd5 to d10e74f Compare July 29, 2019 12:21
@irgolic irgolic requested a review from ales-erjavec July 29, 2019 12:26
@irgolic irgolic force-pushed the yaml-notifications branch from d10e74f to 7185bcc Compare July 29, 2019 13:23
"""
Return the currently displayed widget.
# get singleton instance
self = cls()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Prefer a

   def __init__(self):
       raise TypeError(
           "`NotificationServer` is a singleton and cannot be instantiated. "
           "`Use NotificationServer.instance()` instead"
       )

    @classmethod
    def instance(cls) -> NotificationServer:
        """
        Return the single instance of the NotificationServer class.

        Returns
        -------
        instance: NotificationServer
        """
        if cls.__instance is None:
            cls.__instance = NotificationServer.__new__(NotificationServer)
            super(NotificationServer, cls.__instance).__init__()
        return cls.__instance

and use a NotificationServer.instance() to get the instance. It's clearer and does not need a metaclass (which is too heavy handed for this).

Does the NotificationServer actually need to be a Singleton. From what I can see it is (first) created in main() and is passed to the first main window from whence it propagates further to new windows. That only leaves thepull_notifications() in __main__ which could easily take the instance created in main() as a parameter.

It is not hard to envision multiple separate queues.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I seem to have missed #3933 (comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ales-erjavec, I think that we could still avoid singletons. If an instance is created in main and simply attached to something (module?) that is accessible anywhere, it would still address my concern.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Like so?

@irgolic irgolic force-pushed the yaml-notifications branch 5 times, most recently from 9ace1cc to 4bfd759 Compare August 7, 2019 10:12
@irgolic irgolic force-pushed the yaml-notifications branch from 4bfd759 to af847bd Compare August 9, 2019 16:48
@irgolic irgolic force-pushed the yaml-notifications branch 4 times, most recently from da882dd to c9d31a6 Compare August 23, 2019 08:12
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 23, 2019

Codecov Report

Merging #3933 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3933   +/-   ##
=======================================
  Coverage   85.26%   85.26%           
=======================================
  Files         385      385           
  Lines       68762    68762           
=======================================
  Hits        58630    58630           
  Misses      10132    10132

@irgolic irgolic changed the title [WIP][ENH] Pull YAML feed of notifications on startup, refactor notifications [ENH] Pull YAML feed of notifications on startup, refactor notifications Aug 23, 2019
@irgolic irgolic force-pushed the yaml-notifications branch from c9d31a6 to 15a36de Compare August 23, 2019 09:31
Orange/util.py Outdated
import warnings

try:
from PyQt5.QtCore import pyqtWrapperType
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

While we (probably?) don't support PyQt4 anymore it would still be better to use AnyQt for imports...

requests
openTSNE>=0.3.0
pandas
pyyaml
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Where is this used?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Found it: __main__.py L26

@irgolic irgolic force-pushed the yaml-notifications branch from 15a36de to efc38ea Compare August 30, 2019 12:42
@lanzagar lanzagar merged commit 5435a3b into biolab:master Aug 30, 2019
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.

4 participants