Skip to content

include OrderedDict import in TimeBoundedLRU example #96962

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

Merged
merged 1 commit into from
Sep 22, 2022

Conversation

Harry-Lees
Copy link
Contributor

Currently, the TimeBoundedLRU example in the OrderedDict recipes is mostly complete, except for importing OrderedDict itself. Adding this missing import allows readers to copy paste for a fully working example.

@bedevere-bot bedevere-bot added awaiting review docs Documentation in the Doc dir skip news labels Sep 20, 2022
@rhettinger
Copy link
Contributor

It used to be that he norm for most examples in the docs is to presume that the class or module being explained is already imported. This saves a lot of distracting clutter and lets the examples focus on the core concept. The context makes it clear where the tool comes from. Of late, people have been adding imports in some places. It's mostly a judgment call where they are needed.

In the example for cached_property , we have:

class DataSet:

    def __init__(self, sequence_of_numbers):
        self._data = tuple(sequence_of_numbers)

    @cached_property
    def stdev(self):
        return statistics.stdev(self._data)

That example would be less readable with a from functools import cached_property and import statistics.

Personally, I would leave the OrderedDict examples as-is, but I don't care strongly enough about it to reject the PR.

@Harry-Lees
Copy link
Contributor Author

It used to be that he norm for most examples in the docs is to presume that the class or module being explained is already imported. This saves a lot of distracting clutter and lets the examples focus on the core concept.

I didn't realise that, it makes sense, thanks. I do actually see the same thing has been done further up the page with namedtuple.

EmployeeRecord = namedtuple('EmployeeRecord', 'name, age, title, department, paygrade')

import csv
for emp in map(EmployeeRecord._make, csv.reader(open("employees.csv", "rb"))):
    print(emp.name, emp.title)

import sqlite3
conn = sqlite3.connect('/companydata')
cursor = conn.cursor()
cursor.execute('SELECT name, age, title, department, paygrade FROM employees')
for emp in map(EmployeeRecord._make, cursor.fetchall()):
    print(emp.name, emp.title)

I just found it confusing for these particular examples given that they already had imports but just for other modules (even in the example above, sqlite and csv are imported, but not namedtuple). As I mentioned above

Adding this missing import allows readers to copy paste for a fully working example.

For the uninitiated (me 😄), looking at the provided example, it's not super clear that things have been omitted so it felt natural to try and execute it verbatim.

For the example immediately below

class MultiHitLRUCache:
    """ LRU cache that defers caching a result until
        it has been requested multiple times.

        To avoid flushing the LRU cache with one-time requests,
        we don't cache until a request has been made more than once.

    """

    def __init__(self, func, maxsize=128, maxrequests=4096, cache_after=1):
        self.requests = OrderedDict()   # { uncached_key : request_count }
        self.cache = OrderedDict()      # { cached_key : function_result }
        self.func = func
        self.maxrequests = maxrequests  # max number of uncached requests
        self.maxsize = maxsize          # max number of stored return values
        self.cache_after = cache_after

[...]

My initial thoughts are that the example follows on from the previous one, so it made sense that any supplementary code (i.e. imports) were omitted.

I don't mind closing this issue, I can't see any point in changing this example given that many other examples follow the same philosophy. However, I wonder what your thoughts on including this:

It used to be that he norm for most examples in the docs is to presume that the class or module being explained is already imported. This saves a lot of distracting clutter and lets the examples focus on the core concept. The context makes it clear where the tool comes from.

or a similar sentiment in the FAQ (or somewhere else more appropriate?), as there's quite a few conventions that people may not be aware of if they aren't subscribed to the mailing list/ discourse. It could be helpful to have some of these in the documentation itself.

@rhettinger rhettinger merged commit ec40353 into python:main Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip issue skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants