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 notifications #34

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

Conversation

dzaytsev91
Copy link

@dzaytsev91 dzaytsev91 commented Sep 2, 2016

Implementation of task complete email notifications.
It seems like the setup.py install doesn't install packages from requirements.txt, I had to fix it because travis shows errors.

You can test it by calling:
pyprind.setup_email(smtp_server, smtp_port, username, password)

@dzaytsev91 dzaytsev91 force-pushed the email-notifications branch 4 times, most recently from 953a8b2 to 32ff9f2 Compare September 2, 2016 12:28
@rasbt
Copy link
Owner

rasbt commented Sep 3, 2016

Thanks a lot, looks great so far. Will give it a more thorough look over the weekend and let you know if I have any suggestions! Btw. do you know if it also works on Windows or is it posix only? (Oh, and btw don't worry about squashing commits, GitHub has this handy squash button now so that I can take care of it upon merging)

screen shot 2016-09-03 at 10 08 42 am

@rasbt
Copy link
Owner

rasbt commented Sep 3, 2016

Hm, not sure why, but it's not sending messages in my case. However, when I just ran a simple test

server_ssl = smtplib.SMTP_SSL("smtp.gmail.com", 465)
server_ssl.login('pyprind@gmail.com', passw)  
msg = "\r\n".join([
  "From: pyprind@gmail.com",
  "To: pyprind@gmail.com",
  "Subject: Some test message",
  "",
  "Test"
  ])
server_ssl.sendmail('pyprind@gmail.com', ['pyprind@gmail.com'], msg)
server_ssl.close()

it seems to work fine. I am not getting any error messages via pyprind with email=True after running the email setup, but for some reason, nothing gets send. There are a few other things I found, and it would be great if you could take a look at it

  • add email parameter also to ProgPercent (currently only in ProgBar)
  • add the email param (mentioned above) to the Class' docstrings
  • add a docstring to the new setup_email function
  • I wouldn't make the pycrypto package a required dependency in the setup.py but add it to the requirements as previously. The reason is that this email feature is optional, and for a barebones-pyprind, some people maybe don't want to install it. Can you please change the requirements section back to how it was set up previously but add pycrypto to requirements.txt?
      package_data={'': ['LICENSE',
                         'README.md',
                         'requirements.txt'
                         'CHANGELOG.md',
                         'CONTRIBUTING.md'],
                    'tests': ['tests/test_percentage_indicator.py',
                              'tests/test_progress_bar.py']},
  • not sure what's better, but I'd prefer having the config files in the users home directory, e.g., ~/.pyprind/pyprind.key rather than adding it to the package folder. The reason is that a user's home directory may be a saver place to store such information, despite it being encrypted. Also, it is a common convention (e.g., .vim, .gitconfig etc.) Something like the following should work on Windows and Posix:
def get_pyprind_config_dir():
    home = os.path.expanduser("~")
    config_path = os.path.join(home, '.pyprind')
    return config_path
  • would be nice if you could also change the version tag to __version__ = '2.9.9dev0'
  • also, I am getting an error in Python 3 (works fine in Python 2.7), would be nice to fix that
    106     config.set('Email', 'password', password)
    107     with open(file_path, 'wb') as f:
--> 108         config.write(f)
    109     with open(file_path, 'rb') as af:
    110         cipher.encrypt(af.read())

/Users/Sebastian/miniconda3/lib/python3.5/configparser.py in write(self, fp, space_around_delimiters)
    914         for section in self._sections:
    915             self._write_section(fp, section,
--> 916                                 self._sections[section].items(), d)
    917 
    918     def _write_section(self, fp, section_name, section_items, delimiter):

/Users/Sebastian/miniconda3/lib/python3.5/configparser.py in _write_section(self, fp, section_name, section_items, delimiter)
    918     def _write_section(self, fp, section_name, section_items, delimiter):
    919         """Write a single section to the specified `fp'."""
--> 920         fp.write("[{}]\n".format(section_name))
    921         for key, value in section_items:
    922             value = self._interpolation.before_write(self, section_name, key,

TypeError: a bytes-like object is required, not 'str'

Despite these little points mentioned above, it looks great so far! I really appreciate the contribution!

@dzaytsev91
Copy link
Author

Thanks a lot for your feedback! I will try to fix all the notes as soon as possible

@rasbt
Copy link
Owner

rasbt commented Sep 3, 2016

Thanks a lot for your feedback! I will try to fix all the notes as soon as possible

Thanks! And no need to rush :)

@pep8speaks
Copy link

Hello @dzaytsev91! Thanks for updating the PR.

@dzaytsev91
Copy link
Author

dzaytsev91 commented Mar 10, 2017

Thanks! And no need to rush :)

Hey, sorry for such a delay I've been busy a bit :) I've made all the changes that you manchioned and it would be nice if you could take a look at it.

About setup_email function, maybe I should write a simple example in README? According to a test with sending an email message I run your simple test and in my case it worked good and sends the email. How do you think what could be the reason why it doesn't work for you?

I would really appreciate any comments!

@rasbt
Copy link
Owner

rasbt commented Mar 12, 2017

No need to apologize! And thanks for the updated PR :). A short example in the Readme (and in the example Jupyter notebook) would be nice! I will give it another try then and take a closer look at the code., Thanks!

@dzaytsev91 dzaytsev91 force-pushed the email-notifications branch from 13fac8d to 247c0b4 Compare March 15, 2017 14:31
@dzaytsev91 dzaytsev91 force-pushed the email-notifications branch 3 times, most recently from 281c481 to 062b383 Compare March 15, 2017 15:36
@dzaytsev91 dzaytsev91 force-pushed the email-notifications branch from 6cccd92 to 37a05b6 Compare March 15, 2017 15:37
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.

3 participants