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

email_subject*: don't fail if SPIDERMON_EMAIL_SUBJECT_TEMPLATE is configured in favor of SPIDERMON_EMAIL_SUBJECT #155

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Toritrae
Copy link

@Toritrae Toritrae commented May 3, 2019

Allow usage of SPIDERMON_EMAIL_SUBJECT_TEMPLATE instead of SPIDERMON_EMAIL_SUBJECT without failing during initialization. If EMAIL_SUBJECT is configured it still has precedence.

Allow usage of SPIDERMON_EMAIL_SUBJECT_TEMPLATE over
SPIDERMON_EMAIL_SUBJECT. If the latter is configured it still has
precedence.
@Gallaecio
Copy link
Member

Gallaecio commented May 3, 2019

👍 by me.

I must say I’m surprised it passes all checks, though 🙂 . I think Spidermon uses Black, and Black’s line length limit is 88.

@Toritrae
Copy link
Author

Toritrae commented May 3, 2019

I played around with line length a bit, seems it makes no difference to black. I reformatted the message regardless.

@rennerocha
Copy link
Collaborator

I know that we don't have tests for the actions (and it is something we need to improve), so would you mind create in this PR a new test file for this action and add some tests to make sure that the lack of one of the subject arguments raise an exception? Other tests for other arguments may be created after that (I don't want a PR handles more than one feature).

@Toritrae
Copy link
Author

No problem, will be some time before I get a shot at this though.

@rennerocha
Copy link
Collaborator

@Toritrae , would you be able to include the tests for your code?

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.

None yet

3 participants