-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat: Add EmailTransportMailjet
to email.py
#1162
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @bb-mausbrand,
thanks for your contributing.
Since you are new, a few words about the syntax:
- We use a linter that runs via GitHub Action, locally you can start it with
pipenv run pep8check
— after you have installed your pipenv (you seem to have done that already). Please correct what he complains about :) - We generally use double quotes (
"
), not single ('
) for strings.
And a few comments on the content, in addition to the other comments:
- Please regenerate the
requirements.txt
once after you have changed packages. Otherwise they will not be in the python wheel later.@phorward We could think about making thepipenv install --dev pipenv requirements --hash > src/viur/core/requirements.txt
mailjet-rest
package an extra. That way it won't be installed if you don't need it. - Please add
conf.email.mailjet_api_key
andconf.email.mailjet_api_secret
to the config object.
EmailTransportMailjet
to email.py
lock versions of new packages in Pipfile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phorward We could think about making the
mailjet-rest
package an extra. That way it won't be installed if you don't need it.
@bb-mausbrand can you please find out how to define extras as dependencies, and maybe tie it together with the pipenv-based tooling (see CONTRIBUTING.md for a short explanation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @bb-mausbrand,
I've found some issues regarding the final pull request. Can you check them out?
remove extra modules from `Pipfile` add extra modules to `setup.cfg` update `requirements.txt`
No description provided.