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

Added size and mtime properties to item #13

Closed
wants to merge 10 commits into from

Conversation

nebukadhezer
Copy link
Contributor

Hi Ryan,

cleaned my fork to be able to get these in
-- added mtime and size properties to item
-- added methods to sequence to get max mtime and size for the sequence object
-- added reindex function can change padding too
Regarding the s3d stuff I need to clean that and get the latest changes in my s3dbranch

Cheers
Johannes

johannes added 8 commits December 16, 2014 21:23
-- added mtime and size properties to item
-- added methods to sequence to get max mtime and size for the sequence object
-- added reindex function can change padding too
@@ -733,7 +805,7 @@ def getSequences(source):
items = sorted(source, key=lambda x: str(x))
elif isinstance(source, str):
if os.path.isdir(source):
items = sorted(glob(os.path.join(source, '*')))
items = sorted(glob(os.path.join(source, '*.*')))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the . as getSequence is returning folders which I dont think it should

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, not sure about this one. This assumes only files with a "." in the name are valid sequences, but that may not always be true. Perhaps this kind of thing should be configurable, however (see issue #5).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been using this to get rid of folders from getSequences.
It should only find files with an extension . ...
In my fork I had this with a keyword (folder=False)
elif type(source) == str and os.path.isdir(source) and folders:
items = sorted(glob(os.path.join(source, '')))
elif type(source) == str and os.path.isdir(source) and not folders:
items = sorted(glob(os.path.join(source, '
.*')))

but maybe it is better than to filter folders out specifically instead of globbing only for files with a .

I ll remove that for now.

@rsgalloway
Copy link
Owner

Thanks, Johannes. These seem like useful updates, along the lines of issue #6. I wish I had started this project following strict PEP8 guidelines, but at this point I think the methods should be named consistently at least, even if they're not PEP8. So, do you mind renaming a couple of these?

get_max_mtime -> getMaxMTime
get_size -> getSize

Also, for doc strings, seems like we are following the convention of double-quotes:

"""some useful docs here
"""

Also, I'd like to add you to the AUTHORS file. How do you want to be credited?

edit: looking forward to the s3d updates

tempSize.append(i.size)
return sum(tempSize)

def reIndex(self, addSub, padding=None, run=False):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're increasing the frame numbers, couldn't this possibly overwrite files? Adding 5 on a range of 1001-1010 would move 1001 to 1006 before 1006 has been moved.
I'd recommend reversing your for loop if the addSub is > 0, and keeping it like it is if addSub < 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep you are right, changed that too.
Thanks a ton for the hint.

-- method Names are camelCase for now
-- had to change item.__dirname to use self.__path as it was missing on the first item, though I do not know why….
@@ -86,11 +86,17 @@ def __init__(self, item):
log.debug('adding %s' % item)
self.item = item
self.__path = getattr(item, 'path', os.path.abspath(str(item)))
self.__dirname = os.path.dirname(str(item))
self.__dirname = os.path.dirname(self.__path)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe someone understands why, but dirname was missing on the last item when using str(item), now when using self.__path it is always present...
and wasnot there a dirname on the sequence object too ?

@nebukadhezer
Copy link
Contributor Author

hey Ryan,

regarding the authors file...
Johannes Hezer johannes.hezer@gmail.com

cheers
johannes

@rsgalloway
Copy link
Owner

Thanks, guys. These updates are useful.

Re-thinking about getSize and getMaxMTime, I wonder if it'd be preferable to keep naming parity with the Item class (sorry, Johannes, I know I asked you to change it to this). That way the accesor is the same regardless if you want the size of an Item or the Sequence, e.g.

>>> seqs = pyseq.getSequences(".")
>>> seqs[2].size
85825547
>>> seqs[2][1].size
121976

I've already made this change locally and will push it to main if you (hopefully) agree.

I'm also wondering what the value of the run arg is in reIndex()... are there cases where you want to rename/index all the sequence items but leave the files untouched on disk? Does it make sense to make these kinds of operations transactional? i.e. queue up a series of operations, then execute them all at once and if there are any problems, then rollback (if possible)? Maybe that's getting a bit too sophisticated at this point.

EDIT: apologies to Johannes

seq.size and seq.mtime  are a properties now
removed run from reIndex
@nebukadhezer
Copy link
Contributor Author

Hey Ryan,

all good, I made the changes too...
removed run from reIndex...

cheers
johannes

@rsgalloway
Copy link
Owner

merged in 8abcd10

@rsgalloway rsgalloway closed this Mar 27, 2015
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