Skip to content

Python 3 followup #20

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.

Sign up for GitHub

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

Jump to bottom
Open
PeterShinners wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Python 3 followup #20

PeterShinners wants to merge 3 commits into from

Conversation

@PeterShinners
Copy link
Contributor

@PeterShinners PeterShinners commented May 25, 2021

Two followup commits for the Python 3 migration. See each commit message for more details.

  1. Under Python 3 various recursive iterations could get a RuntimeError when the hierarchy is changed during iteration. There was a mention of this at the end of the previous Support for Python 2 and Python 3 #19 .
  2. Unit tests now work with Python 3 and under Windows. The changes were only needed for testCask.py, the cask library itself was working as-is.

@PeterShinners @pshinners-blz
Handle Python 3 iterating over modified dictionaries
f596b11 
Under Python 3, the dictionary values() call gives a view
object that raises RuntimeError if the dictionary is modified
while iterating. Technically that also happened under Python 2
if the dictionary itervalues() method was used.

This uses a simple _copiedvalues() function to get a standalone
list of values efficiently on either version of Python. The
recursion functions for find, save/write, and close use this
to prevent exceptions. The behavior under Python 2 is unchanged.
@PeterShinners PeterShinners mentioned this pull request May 25, 2021
Open
@ArlonSwaders
Copy link

ArlonSwaders commented Jan 19, 2023 •
edited
Loading

@PeterShinners Thank you very much for following up on this.

Cask has been a great help for us to simplify operations on Alembic files. We like it a lot and have Cask as a dependency for a number of our production tools. We are in the progress add Python 3 support throughout our pipeline to be compatible with VFX Reference Platform CY2020 and up.

@rsgalloway Any chance this PR can be merged? That would help us a great deal and I'm sure many more.

Thanks in advance.

@rsgalloway
Copy link
Collaborator

rsgalloway commented Jan 24, 2023

hey all, thanks for the reminder on this.

  1. this looks like it would work fine, but personally I'd rather see an update to the DeepDict class (e.g. overriding values to return a List) since I believe both child objects and properties are using this class. seems like there might be a cleaner solution there.

  2. just like the main alembic repo, before merging,I believe the contributors agreement must be signed and submitted: https://github.com/alembic/alembic/wiki/Alembic-Contributor-License-Agreements

@PeterShinners
Copy link
Contributor Author

PeterShinners commented Jan 29, 2023

Good feedback, I'll take a pass on the code changes today. I believe I have a personal CLA with the Alembic project, but if I do I set that up many years ago. I'll see what I can find.

@PeterShinners
Unit tests work with Python 3
346349f 
This requires no changes to the cask library itself. But the testing
needed a few changes to work on Python 3. This also updates the
tests to pass on Windows.
@PeterShinners
Cleaner python 3 dictionary values
783c9c3 
The cleanest way for dictionary.values() to match the Python 2
behavior is to override the DeepDict.values() method directly.
@PeterShinners
Copy link
Contributor Author

PeterShinners commented Jan 29, 2023

I couldn't find any records of a previous Personal CLA. I just submitted a new agreement to the contributor's email but got an error about a missing account.

Final-Recipient: rfc822; contributors@alembic.io
Action: failed
Diagnostic-Code: smtp; The email account that you tried to reach does not exist. Please try double-checking the recipient's email address for typos or unnecessary spaces.

@rsgalloway
Copy link
Collaborator

rsgalloway commented Jan 31, 2023

I couldn't find any records of a previous Personal CLA. I just submitted a new agreement to the contributor's email but got an error about a missing account.

Final-Recipient: rfc822; contributors@alembic.io
Action: failed
Diagnostic-Code: smtp; The email account that you tried to reach does not exist. Please try double-checking the recipient's email address for typos or unnecessary spaces.

Interesting. I assumed this document is still current:
https://github.com/alembic/alembic/wiki/Alembic-Contributor-License-Agreements

Tagging @lamiller0 for more info.

@lamiller0
Copy link
Contributor

lamiller0 commented Jan 31, 2023

Pete is still covered by a corporate CLA (as I dont believe he has recently changed jobs)

FYI: For new personal CLA I've been having them email it to me directly to forward onward.

@rsgalloway
Copy link
Collaborator

rsgalloway commented Jan 31, 2023

Pete is still covered by a corporate CLA (as I dont believe he has recently changed jobs)

FYI: For new personal CLA I've been having them email it to me directly to forward onward.

Thanks, @lamiller0 good to know.

@PeterShinners is the _dictvalue function in the unit tests here still necessary after the change to DeepDict?

@PeterShinners
Copy link
Contributor Author

PeterShinners commented Jan 31, 2023

The _dictvalue is only for the testing. This could be similarly added onto the DeepDict but I'm not sure what I'd call it, getOneArbitraryValue. But since its only used in the unit tests I'm comfortable leaving it where it is. (PS, I have not changed jobs)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@PeterShinners @ArlonSwaders @rsgalloway @lamiller0