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

Incidental implementation specific types #472

Merged
merged 5 commits into from
Jun 10, 2022

Conversation

anivegesana
Copy link
Contributor

#449 (comment)

Extended the list of inaccessible Python types to include types that were added as recently as Python 3.11.

dill/_dill.py Outdated Show resolved Hide resolved
Copy link
Member

@mmckerns mmckerns left a comment

Choose a reason for hiding this comment

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

All of the objects currently in _reverse_typemap are also later fed to register (with the exception of the ExitType). I believe that's primarily why some objects were in the _reverse_typemap, and others were not. You've added a number of objects... and I assume none have been registered because pickle will handle those objects without intervention from dill. If that's so, then what's the point of adding them to the _reverse_typemap? If it's just to have them included in dill, so one can grab an object of that type if needed... then I'd argue they could just be added to dill._objects and left out of _reverse_typemap. Otherwise, LGTM.

@mmckerns mmckerns added this to the dill-0.3.6 milestone Jun 5, 2022
@anivegesana
Copy link
Contributor Author

I am still on the fence about including the types themselves in the reverse typemap. I think it makes perfect sense to put them in objects.py because the objects are commonly used by Python users, but the types themselves are more of an internal detail that I don't think anyone uses directly. PyPy and CPython iterate over builtin collections differently leading to differences there which make it a little bit more confusing.

@mmckerns
Copy link
Member

mmckerns commented Jun 8, 2022

If it helps, I can figure out what exactly my rules are for adding something to _reverse_typemap. I can tell you that very generally in the past a type has not been added if the pickle Pickler handled it without dill(i.e. dill did not need to register it).

@anivegesana
Copy link
Contributor Author

That definition is a little overloaded. XRangeIteratorType objects can be pickled but the type XRangeIteratorType itself cannot. Don't think that anyone should pickle it, but it would make _reverse_typemap incomplete. I'll remove it and if anyone needs it they can open an issue and add it later.

@mmckerns
Copy link
Member

mmckerns commented Jun 9, 2022

Let me know when you want me to review this again.

@anivegesana
Copy link
Contributor Author

I think it is good now. Don't know why the coverage went down when I removed code. I will see if it is something that I did.


try:
import symtable
_incedental_reverse_typemap["SymtableStentryType"] = type(symtable.symtable("", "string", "exec")._table)
Copy link
Member

@mmckerns mmckerns Jun 9, 2022

Choose a reason for hiding this comment

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

Why SymtableStentryType as opposed to SymtableEntryType? ...STring Entry?

Copy link
Member

@mmckerns mmckerns left a comment

Choose a reason for hiding this comment

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

Looks good. Please address the question with regard to the name SymtableStentryType.

@anivegesana
Copy link
Contributor Author

I called it SymtableStentryType because that is what the name of the type is (before changing from snake case to camel case.) I will change it to SymtableEntryType to be a little more consistent.

@mmckerns
Copy link
Member

The few places I found references to it it was as SymtableEntryType, so I prefer that. Thanks.

@mmckerns mmckerns merged commit 0ce3baf into uqfoundation:master Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants