gsilvapt commented 1 year ago

Input

The slack-sdk dependency is set as an extra, although running the install step in a fresh installation returns errors due to the missing dependency. In my perspective, this should suffice to move this dependency to required, instead of extras, because this will require users to install the dependency to simply run the test suite.

make test output in a fresh environment, after running python setup.py install:

» make test                                                                                               130 ↵
testapp/manage.py test ${TEST_ARGS:-tests}
Found 11 test(s).
Creating test database for alias 'default'...
System check identified some issues:

WARNINGS:
database_locks.Lock: (models.W042) Auto-created primary key used when not defining a primary key type, by default 'django.db.models.AutoField'.
        HINT: Configure the DEFAULT_AUTO_FIELD setting or the DBLocksConfig.default_auto_field attribute to point to a subclass of AutoField, e.g. 'django.db.models.BigAutoField'.

System check identified 1 issue (0 silenced).
EE.E.......
======================================================================
ERROR: tests.test_blocks (unittest.loader._FailedTest.tests.test_blocks)
----------------------------------------------------------------------
ImportError: Failed to import test module: tests.test_blocks
Traceback (most recent call last):
  File "/Users/silvag3/.pyenv/versions/3.11.1/lib/python3.11/unittest/loader.py", line 407, in _find_test_path
    module = self._get_module_from_name(name)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/silvag3/.pyenv/versions/3.11.1/lib/python3.11/unittest/loader.py", line 350, in _get_module_from_name
    __import__(name)
  File "/Users/silvag3/github.com/surface-security/django-notification-sender/testapp/tests/test_blocks.py", line 2, in <module>
    from slack_sdk.errors import SlackApiError
ModuleNotFoundError: No module named 'slack_sdk

https://github.com/surface-security/django-notification-sender/blob/2f558a31af7abbffe9b4fcf2a46342ecc5408a38/setup.cfg#L35-L36

Output

Move the slack dependency definition to the install_requires block.

gsilvapt commented 1 year ago

@fopina could you give your 2 cents on this, please?

fopina commented 1 year ago

👋 I think when it was initially added to optional it was so that anyone using email only would not require the huge dependency list from slack-sdk

But I suppose eventually that optional was forgotten and code is not prepared to not having slack.

Ideally there would be a class wrapping each "transport" (including the block abstraction), but that was never done.

Also, it would be nice to be able to implement new transports outside of the app and, given that, slack could moved to a separate module so it would rely on these optionals.

TL;DR; LGTM 😀

surface-security / django-notification-sender

Slack should be in `install_requires` #9

Input

Output