Allow passing of Python objects in the parameters dict

[connorjward]

We build an appctx manager under the hood to coordinate things.

[JHopeCollins]

This is much more ergonomic. Is there a benefit to still allowing passing an AppContextManager? Could we just make this an internal detail and have One True Way of passing python types?

[connorjward]

This is much more ergonomic. Is there a benefit to still allowing passing an AppContextManager? Could we just make this an internal detail and have One True Way of passing python types?

You make a good point. I was torn as to whether we still want the class around. It is a nice abstraction but I think I agree that we should make it purely internal. Will do.

[connorjward]

@JHopeCollins I think I'm now happy with this.

[JHopeCollins]

Really really nice. Big fan.

Will go through in more detail, but at a first pass one thing that jumps out is that we should override Options.__additem__ in case someone tries to do the following:

petsctools.Options["key"] = python_object

The easy thing is just to disallow it because there's no way to associate an AppContextManager. Unless we think it would be safe to just stick it into the global appctx dictionary...

[connorjward]

Really really nice. Big fan.

Will go through in more detail, but at a first pass one thing that jumps out is that we should override Options.__additem__ in case someone tries to do the following:

petsctools.Options["key"] = python_object

The easy thing is just to disallow it because there's no way to associate an AppContextManager. Unless we think it would be safe to just stick it into the global appctx dictionary...

I think inserting it into the global appctx dictionary would be the right thing to do

[JHopeCollins]

Other than some docstring updates this looks great.

[connorjward]

Really really nice. Big fan.
Will go through in more detail, but at a first pass one thing that jumps out is that we should override Options.__additem__ in case someone tries to do the following:

petsctools.Options["key"] = python_object

The easy thing is just to disallow it because there's no way to associate an AppContextManager. Unless we think it would be safe to just stick it into the global appctx dictionary...

I think inserting it into the global appctx dictionary would be the right thing to do

I've just added __setitem__ and __delitem__ plus tests for them.

[JHopeCollins]

Should fix the CI errors, then this is good to go

[connorjward]

Sweet. I'll push it through from here. Thanks for all the reviews!