On 15May2014 15:34, Benjamin Schollnick <benja...@schollnick.net> wrote:
I am going to be using this code as part of a web system, and I would love
any feedback, comments and criticism. [...]
I am using scandir from benhoyt to speed up the directory listings, and
data collection. [...]
I had considered using OrderedDicts, but I really didn't see how that would
help the system.

I'm not completely happy with the return_sort_* functions, since they
return two different tuples, one goal was to try to keep everything in the
dictionary, but I couldn't think of a better method.

So any suggestions are welcome.

Firstly, I'm with ChrisA on the whole: let the filesystem/OS do the stat caching. It _is_ actually slower to rely on this than an in-memory cache such as yours, but at least it is reliable because you will then have exactly the same file info state as any other user of the OS. Stale caches are a usability PITA for the end user; witness the continued presence of "Shift-Reload" in most browsers because browser (and proxy) caches get stale.

His remark about relying on the mtime of a directory is correct also, and not just because of clock changes. On UNIX, the mtime of a directory changes _only_ when a filename is added/removed from the directory. To the point, it will not change if a file in the directory is just modified, and therefore your cache will become stale (and stay stale) in that circumstance.

Of course, you wouldn't even be bothering with scandir if you were not concerned about relying of the filesystem/OS. So, it is your call about remembering this stuff. I would include a real-time staleness in addition to the mtime check, eg if the mtime is updated _or_ the system time says it is more than N seconds since the last directory scan, with N being smallish.

Most of my other remarks have to do with style and implementation.

Preqs -
   Scandir - https://github.com/benhoyt/scandir
   scandir is a module which provides a generator version of
   os.listdir() that also exposes the extra file information the
   operating system returns when you iterate a directory.

Personally, my habit is to use os.listdir() and accept the memory cost for two reasons: it is simple and the list is stable. If you iterate over a directory (eg via the C library readdir() facility) to my mind there is scope for the directory changing underneath you. A bit like iterating over a dictionary which is in active use by other code. Using os.listdir() minimises that window.

So I'd be going:

  names = list(scandir(directory_path))

to get a snapshot, and then working with that list.

from stat import ST_MODE, ST_INO, ST_DEV, ST_NLINK, ST_UID, ST_GID, \
                   ST_SIZE, ST_ATIME, ST_MTIME, ST_CTIME

You don't need these. os.lstat() and os.stat() return stat objects, and they have convenient .st_mtime etc attributes. So all your:

  data["st_nlink"] = st[ST_NLINK]

stuff can become:

  data["st_nlink"] = st.st_nlink

I'd also be making your "data" object just an object, and assigning directory to an attribute on it, thus:

  data.st_nlink = st.st_nlink

Much more readable.

You could also avoid the entire issue of manually copying each st.st_blah attribute by just going:

  data.st = st

Then just reach for data.st.st_mtime (or whatever) as needed. Short, quick, avoids needing to worry about optional stat fields - just keep the original stat result.

import time
import scandir

plugin_name = "dir_cache"

class   CachedDirectory(object):
   For example:

       To be added shortly.

It is _well_ work sketching this part out. Since this is just a cache, an example should be quite short.

I find it enormously convenient to sketch some of the use cases here - it helps keep the required method names nicely defined. i.e. sketch a little code that you need to write, then write methods to support the code. You get a much better match that way. I often revisit classes when I use them, because I write some code using the class and think "that was very painful, how would I _like_ to have been able to use this"?

   def __init__(self):
       self.files_to_ignore = ['.ds_store', '.htaccess']
       self.root_path = None

You don't seem to use .root_path?

           # This is the path in the OS that is being examined
           #    (e.g. /Volumes/Users/username/)
       self.directory_cache = {}

This is a CachedDirectory class. I would have just called this .cache - it will make all the code using it less wordy, and hopefully more readable. Unless there may be caches of other stuff too. This is almost entirely a matter of personal style though.

   def _scan_directory_list(self, scan_directory):
           Scan the directory "scan_directory", and save it to the
           self.directory_cache dictionary.

           Low Level function, intended to be used by the populate
       scan_directory = os.path.abspath(scan_directory)
       directories = {}
       files = {}
       self.directory_cache[scan_directory.strip().lower()] = {}
       self.directory_cache[scan_directory.strip().lower()]["number_dirs"] = 0

You write "scan_directory.strip().lower()". It if probably worth going:

  norm_directory = scan_directory.strip().lower()

and saying "norm_directory" throughout. Also, you say:


You can also bind:

  cache = self.directory_cache[norm_directory]

right after binding "norm_directory", and say "cache" throughout instead of the longer term. It is naming the same object.

self.directory_cache[scan_directory.strip().lower()]["number_files"] = 0
       for x in scandir.scandir(scan_directory):
           st = x.lstat()
           data = {}

As mentioned earlier, you could usefully define a tiny class:

  class Dirent(object):

and go:

  data = Dirent()

which will allow you to replace:

           data["fq_filename"] = os.path.realpath(scan_directory).lower() + \


  data.fq_filename = os.path.realpath( os.path.join(norm_directory,
                                                    x.name.strip().lower()) )

Note also the use of os.path.join: it is portable across OSes (eg UNIX versus Windows).

Also, well worth defining:

  norm_name = x.name.strip().lower()

and just saying "norm_name" throughout. More readable, less prone to typing accidents.

Um. A serious point here: why do you go .strip().lower() to all your filenames and pathnames? On a real UNIX system that will usually mean a different object. Many other platforms may have case insensitive names, but will still complaint about names with and without whitespace as different objects. I would even expect os.path.realname to be unreliable if handed such mangled names.

If you are sanitising some input I would recommend doing that elsewhere, and not binding some much special knowledge into this class.

Maybe you have good reasons for that. At the least those reasons need to be expressed in comments (best in the opening docstring), because otherwise I would recommend strongly to make the cache class as naive about names as possible (i.e. do not mangle them).

           data["parentdirectory"] = os.sep.join(\

Use os.path.dirname.

           data["st_mode"] = st[ST_MODE]
           data["st_inode"] = st[ST_INO]
           data["st_dev"] = st[ST_DEV]
           data["st_nlink"] = st[ST_NLINK]
           data["st_uid"] = st[ST_UID]
           data["st_gid"] = st[ST_GID]
           data["compressed"] = st[ST_SIZE]

"compressed" is worth a comment.

           data["st_size"] = st[ST_SIZE]       #10
           data["st_atime"] = st[ST_ATIME]     #11
           data["raw_st_mtime"] = st[ST_MTIME] #12
           data["st_mtime"] = time.asctime(time.localtime(st[ST_MTIME]))

I strongly recommend against this.

Keep "st_mtime" (or data.st.st_mtime per my earlier suggestion) as the raw mtime. Any human readable transcription of the time should get a name like "human_st_mtime".

Better still, if you make a Dirent class for "data", you can give it a property like:

  def human_st_mtime(self):
    return time.asctime(time.localtime(self.st.st_mtime))

and just use ".human.st.mtime" like any other attribute later when needed. Note: no brackets.

           data["st_ctime"] = st[ST_CTIME]
           if not x.name.strip().lower() in self.files_to_ignore:

Personally I phrase this as "a not in b" instead of "not a in b".

Also, if you really are ignoring it, you could go:

  if norm_name in self.files_to_ignore:
    # do nothing else with this entry

and save yourself some indentation below. Not everyone likes this.

   def directory_in_cache(self, scan_directory):
           Pass the target directory

           Will return True if the directory is already cached
           Will return False if the directory is not already cached
       scan_directory = os.path.realpath(scan_directory).lower().strip()

Probably worth making normalisation function:

  def norm_name(name):
    return name.lower().strip()

becuase you do this a lot. Using a function ensures you do it the same way everywhere and makes it obvious that it is the same thing with the same purpose everywhere.

       return scan_directory in self.directory_cache.keys()

You can just say:

  return scan_directory in self.directory_cache

and for added points you can rename "directory_in_cache" to "__contains__". That way from outside (or inside) you use "in" directly. Example:

  if "foo" in self:

Cameron Simpson <c...@zip.com.au>

Reply via email to