More informative GraphML exceptions

[rossbar]

Closes #5024

Modifies how the dict that defines supported GraphML types is accessed so that an informative exception is raised if a user tries to save an attribute of an unsupported type.

This solution is technically backward incompatible, as it changes the (currently public) xml_types attr of the GraphML class from a dict to a method. The reason for doing it this way is that it provides an easy way to ensure that the exception is raised for all of node, edge, and graph attrs. If this is a problem though, we can always go with the clunkier (but backward compatible) approach which would add try-excepts each time the dict is accessed. LMK what you think!

[dschult]

Nice approach! -- I like it.

Would it help backward compatibility if we could tell people (who have adapted the public xml_types for their problem say to a dict new_xml_types) that they can replace the new public xml_types function using new_xml_types.__getitem__? Does that actually make it work like the old code?

I'm not actually sure we want to advertise this way of doing things, but have I understood the backward compatibility issue? Are there other things people may have used the public xml_types for? I guess it is hard to know....

I'm definitely good for leaving it as you have proposed. I'm not sure a deprecation is even needed. Maybe a release note and a discussion of the extent of backward incompatibility here in this PR will be sufficient....??..

[rossbar]

Thanks for the feedback @dschult , you're right that there's actually a very straightforward way to do this without breaking compatibility by simply changing the naming scheme around a little bit. The way I've done it in 8a739f9 will break anyone who is modifying/extending/inheriting from GraphML class downstream if they're expecting xml_types to be a dict. I was hung up on the access pattern where there is a private attribute (with an underscore) and a public method/property of the same name (without underscore) that implements access. AFAICT from reading other code this is a common pattern, but it's really just convention.

I've changed the naming around a bit in 42968da to avoid this problem. Adding a method that starts with get_ feels unPythonic but... practicality over purity

[dschult]

Clever! :}
I approve this PR.