Scrape a reddit post submission for comments and save comments to JSON file

Question

Edit: For a revision to this code according to the comments, see here: "here."

In regards to the code involving shortlinks and submission:
this script is meant to be used in conjunction with a different script not displayed here, which initializes the data.json with the appropriate keys. Code related to submission, comment.list(), praw all deal with the reddit API.

How it works: Retrieves post information from data.json, using the shortlinks as reference, scrapes the aforementioned post for all of its comments, and then matches the group of comments to its corresponding shortlink in order to write the information to the JSON file.

#!/usr/bin/python3
import json
import os
import praw

"""
Scrapes posts for comments and saves the comment ID and comment text to the
accompanying JSON file.
"""

JSON_DEFAULT_LOC = os.environ.get('JSON_LOCATION') or os.path.join(
    os.path.dirname(os.path.dirname(os.path.abspath(__file__))), "data.json")


def login():
    """ Create a reddit instance according to praw.ini configuration.
    Call reddit.user.me() as a check.
    Return reddit instance.
    """
    print("Authenticating")
    reddit = praw.Reddit("appname", user_agent="Chrome:com.example.bot-"
                                                "name:v1 (by /u/)")
    print("Authenticated as {}".format(reddit.user.me()))
    return reddit


def load_manager(fp=None):
    """ Load the accompanying JSON data which holds reddit post information.
    """
    if fp is None:
        fp = JSON_DEFAULT_LOC
    with open(fp, "r") as file:
        data = json.load(file)
    return data


def scrape(reddit):
    """ For each post specified in the JSON dict, scrape the comments within
    the post and write the comments to the JSON dict to the key: "responses".
    """
    data = load_manager()
    shortlinks = [data[sub][0]["shortlink"][-6:] for sub in data]

    for sl_id in shortlinks:
        submission = reddit.submission(id=sl_id)
        try:
            write_comments_to_file(submission, sl_id, data)
            print("Comments from '{}' "
                  "saved to file".format(submission.subreddit))
        except Exception as e:
            print("Could not write to file: {}".format(e))


def write_comments_to_file(submission, sl_id, data, fp=None):
    """ Takes a reddit submission object, shortlink ID, and JSON data.
    Retrieves the comments per submission, and for each submission within the
    JSON data, also write its comments.
    """
    submission.comments.replace_more(limit=0)
    cmt_data = [{comment.id: comment.body}
                for comment in submission.comments.list()]
    for i in data:
        if sl_id in data[i][0]["shortlink"]:
            data[i][1]["responses"] = cmt_data
    if fp is None:
        fp = JSON_DEFAULT_LOC
    with open(fp, "w") as fp:
        json.dump(data, fp)


def main():
    reddit = login()
    scrape(reddit)


if __name__ == "__main__":
    main()

I am interested in making the code more Pythonic. I'm especially unconfident about the way that my functions are structured/grouped.
Help much appreciated!

Answer 1

First of all, naming: do not abbreviate names unless they are used in a one-liner like [x.something for x in somelist]. Do not try to spare characters - use IDE to suggest continuations if you want to type fast. Really pays off.

Second, try not to mix abstractions of different types in the same function, eg, write_comments_to_file deals now with preparing content as well as low-level detail of writing to file, extract to a function with meaningful name - will make code more readable. Try to avoid names with data, info, etc in it - they are useless. Everything is data.

The same function seems to make changes to submission - if it is the case, make it explicit and in a separate function: submission_replaced = replace_more(submission) (good names here as well - I just tried to guess one).

All that is needed for the code to show the intention behind it. except Exception as e is too broad. What exceptions you really want to catch? Maybe, only IOError is enough?

Magic numbers: -6 <- why six? Extract that to a new function with a meaningful name like strip_html_postfix (or whatever is the intention of that) - then, there is no need to leave a comment, function name will serve as a comment. Using appropriate names can lessen the need to document the code, even to the point that no separate docstrings will be necessary.

You have not told whether this is one-time script or part of bigger system, so I will not touch possible use of classes. For example, propagation of fp (is it filepath?) and if for missing fp may be a signal, that the code may benefit from using a Loader class (omitting manager here, because it is usually as useless as data in class names unless correspond to manager in real world). For example, I see classes: Loader, Scraper, and Outputter which will nicely decouple the load manager configuration, process of scraping and saving the results to whatever destination - but of course it's overkill for a script, and is useful only if you foresee more Loaders, Scrapers or Outputters.

No matter of the programming language, functions are always more readable when they have explicit arguments and return results they produce. For instance, scrape at the moment uses module-level data - it's harder to write tests, configure, and guess what it does.

Hopefully, my findings above will help you write code, which will be self-explanatory and will not require the How it works you provided. This would be ultimately Pythonic.

PS. for ... data[i][1]["responses"] = cmt_data - this is dangerous in Python if you intend to use the result beyond converting to JSON. For example, if the data structure will be mutated, then it will be mutated for all responses (cmt_data is a pointer, not value, so to say!), which is hardly what is desired. And in this case data is from outside, so you may already have a problem elsewhere in the code and spend time to debug, because it will not be immediately clear the function in question make your responses entangled. Use copy or deepcopy, and overall it's not good idea to mutate data structure in a function without explicitly returning it back, and even then assignment of cmt_data can be frowned upon.