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

Slack should be in install_requires #9

Open
gsilvapt opened this issue Jan 17, 2023 · 2 comments
Open

Slack should be in install_requires #9

gsilvapt opened this issue Jan 17, 2023 · 2 comments
Labels
bug Something isn't working enhancement New feature or request

Comments

@gsilvapt
Copy link
Contributor

gsilvapt commented Jan 17, 2023

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

[options.extras_require]
slack = slack_sdk==3.11.2

Output

Move the slack dependency definition to the install_requires block.

@gsilvapt gsilvapt added bug Something isn't working enhancement New feature or request labels Jan 17, 2023
@gsilvapt
Copy link
Contributor Author

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

@fopina
Copy link
Contributor

fopina commented Jan 17, 2023

👋 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 😀

gsilvapt added a commit that referenced this issue Jan 19, 2023
As discussed in #9, this dependency should be in required dependencies,
otherwise users will have to manually install the dependency to have the
project running.

Closes #9.
@fopina fopina mentioned this issue Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants