Add suffix option #4
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merged
Lee-W
requested changes
May 18, 2020
Member
Lee-W
left a comment
Lee-W
left a comment
There was a problem hiding this comment.
Thanks for your contribution! I leave some discussion regarding the try catch block. Also, I'll try to apply new tools in this repo. You might need to pull the latest version from this repo after that.
tai271828
force-pushed
the
pr-render-mail-config
branch
from
11342a9 to
e6ba004
Compare
Member
Author
|
The PR is ready for review again except one coding style error This updated PR includes:
The implementation of the test cases has a lot of improvement space. It was implemented on top of the data structure that is out of scope. We may want to use this imperfect test as a foundation for the possible refactoring in the future rather than a very large code change for review (this PR is large enough, right? ; ) ) Verify This PRBy More Tests Were Performedpytest tox Render and send the mails |
Lee-W
requested changes
May 26, 2020
Member
Lee-W
left a comment
Lee-W
left a comment
There was a problem hiding this comment.
Thanks for your contribution! It's a huge improvement in our existing codebase. I like the tox integration to the project.
I already fix the Github action. Two of our tests fail. You can view them here.
As for the Imports are incorrectly sorted. discussion, we use isort to sort our imports. You can use inv style.reformat to reformat the codebase. Aside from following what pep8 suggests, it sort libraries in the same group in alphabetic order.
I did not notice the last part of message regarding to the test part. Surely, we can fix them later if you deem they are too huge to fix in this pull request
tai271828
force-pushed
the
pr-render-mail-config
branch
from
b1d9753 to
0b3b698
Compare
Lee-W
reviewed
May 26, 2020
Member
Lee-W
left a comment
Lee-W
left a comment
There was a problem hiding this comment.
Other than the failed tests, there's no major change needed. Once they're fix, we can merge this PR. 🎉
tai271828
force-pushed
the
pr-render-mail-config
branch
from
0b3b698 to
a19516a
Compare
We already have flake8 check via invoke and only one flake8 configuration in one place will make the code easier to maintain.
By removing the ephemeral files we may avoid false positive testing result in the future, because the runner may not generate expected files for each regression test.
tai271828
force-pushed
the
pr-render-mail-config
branch
from
c32921c to
189e475
Compare
Member
Author
|
Besides CI, sending rendered emails via gmail was also performed (again) |
Lee-W
requested changes
May 26, 2020
send_mail.py
Outdated
|
|
||
| import click | ||
|
|
||
| from tests.utils import send_mail_debug_dump_path |
Member
There was a problem hiding this comment.
It seems to me that our program should not depend on our test files. Even duplication seems to be a better solution to me. One of the reasons is that after we make this project a package, it won't pack tests into it. @tai271828 What do you think?
tests/utils.py
Outdated
Comment on lines
17
to
21
| all_mail_names = [] | ||
| for mail in mails: | ||
| all_mail_names.append(os.path.basename(mail)) | ||
|
|
||
| return all_mail_names |
Member
There was a problem hiding this comment.
I prefer using list comprehension for simply iterations. i.e. return [os.path.basename(mail) for mail in mails]
Member
Author
There was a problem hiding this comment.
+1. We should always insist list comprehension. This is suggested in "fluent python".
Lee-W
approved these changes
May 27, 2020
Member
Lee-W
left a comment
Lee-W
left a comment
There was a problem hiding this comment.
Thanks for all the hard-working! Let's merge it!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By adding suffix option we could then appending suffix to the original email subject. This will help us separate the corresponding threads of gmail. Gmail is happy to group emails with the same subjects, but in the scenario of how the sponsorship team organizes the emails, the sponsorship team would like to handle each email separately rather than folding them in the same thread.
Steps to Test This PR
The following steps are also performed and verified.
The Rendered Contents Are The Same
And diff the files in
mails_to_sentExpected Result
No difference shown by the diff command.
Send The Rendered Emails
Expected Result
The receivers could get the email with the render contents.
More Information
Please note I did not upload the corresponding testing json files because #1 has not merged. Touching the same json files will raise conflicts. I could upload them later after #1 is merged.