datactive / bigbang

Scientific analysis of collaborative communities
http://datactive.github.io/bigbang/
MIT License
154 stars 51 forks source link

Self-contained LISTSERV 16.5 classes #409

Closed Christovis closed 3 years ago

Christovis commented 3 years ago

This PR does several things:

  1. makes isort compatible with black. Otherwise they change each others changes.
  2. add a ignore flag to flake8 for lambda functions. The problem is that python gives them all the same name which can cause troubles to identify them while debugging. But as I implemented them only in one place, it isn't a problem for us.
  3. add listserv functions to deal with listserv mailing lists in local files and web scraping. Test are included in .tests/test_listserv.py
sbenthall commented 3 years ago

The stacktrace from when I try to run the tests. Looks like a bs4 dependency issue.

May well be an environment configuration issue on my end.

What happens if you try pip freeze ?

https://gist.github.com/sbenthall/ce3f3154a8de2fb15575dc4565abbdd9

sbenthall commented 3 years ago

Dependencies:

https://gist.github.com/sbenthall/1cb329eaf3422ae3dd5c27b947b15fa3

Christovis commented 3 years ago

I think it is a problem with the BeautifulSoup version. But I am a little confused about the difference of bs4 and beautifulsoup4... (I think there is none). I have the following: bs4 == 4.9.3 if it's correct than you have: bs4 == 4.6.0 and after doing: pip install --upgrade beautifulsoup4 it hopefully works.

sbenthall commented 3 years ago

Automated tests are failing for me.

https://gist.github.com/sbenthall/0b75d0e117a9dd13b566e7f71704a647

The issue appears to be that the tested code calls Python's input() as a query for a user's email address.

An automated test should not depend on user input.

One way to get around this is to mock up the user input and built the test around the mockup. See this StackOverflow entry:

https://stackoverflow.com/questions/51392889/python-pytest-occasionally-fails-with-oserror-reading-from-stdin-while-output-i

But I'm a little skeptical of a design that requires user input(). It would be more conventional for the credentials to be entered into a local configuration file.

Christovis commented 3 years ago

Hmm... as can be seen in the tests, I do create a mock.

As Niels and I have discussed in the Email conversation, it is good to have the option to have a prompt for it during run-time in case a authentication keys could be found in the location they should be, and saved there. So there are both option: 1) a local configuration file 2) run-time user input

Christovis commented 3 years ago

Just added a small fix that avoids asking for user input when it isn't tested, but does for the test that needs (and mocks) it.

sbenthall commented 3 years ago

Thanks for the fix!

I see what you mean about the value of using input().

What I would have done, in terms of design, is to use input() in a collection script in bin/, but kept it out of the library code.

The idea being that bin/ is part of the "user interface" of BigBang, whereas the library code should at least in principle be useful as a dependency in other applications. It would be pretty weird to install BigBang as a dependency, and then have it trigger an input when one of its internal modules was used.

I don't think this issue should block this PR, but I'd like to make a ticket noting it as a future adjustment, if you don't mind.

sbenthall commented 3 years ago

I had to upgrade my lxml dependency separately to get this working. I wonder if it needs to be added explicitly to the requirements.

sbenthall commented 3 years ago

Thanks for the great work @Christovis I'm merging now;