issue_comments

Unit Test Results

0 files  ±0  0 suites  ±0   0s :stopwatch: ±0s 0 tests ±0  0 :heavy_check_mark: ±0  0 :zzz: ±0  0 :x: ±0 

Results for commit e87d65b7. ± Comparison against base commit e87d65b7.

thanks for the reviews, @shoyer, @dcherian, @max-sixty

let's merge this as-is

Works for me. Once merged perhaps @huard or @DamienIrving can help us iterate on context

let's merge this as-is (unless there are any comments on the current state?) and I'll add the construction of the context objects in a new PR.

Also, how do we best construct that object without a lot of overhead? We need to get at least the function name, but if we pass that manually it's one more place to update when renaming something (not that we do that very often). Using inspect is possible but might be too complicated:

Rather than introspection, I think we should try to be fully explicit about the function being called. Trying to introspect it from stack-frames is madness :)

So in that case, we would need to pass down the context information from the top level functions in xarray, e.g., everything that takes a combine_attrs argument.

In terms of the overall interface, one other concern I have is about the information we make available to users of this API. I can imagine that they might not only want attributes but also the complete xarray objects on which the function is being called. If that's the case, then they would also need more information from the global context.

there's two remaining issues: should we use a namedtuple / a dataclass for Context instead of the custom class? We basically want a struct without methods (for now?).

Also, how do we best construct that object without a lot of overhead? We need to get at least the function name, but if we pass that manually it's one more place to update when renaming something (not that we do that very often). Using inspect is possible but might be too complicated: python In [5]: import inspect ...: ...: def current_function_name(): ...: frame = inspect.currentframe() ...: try: ...: caller = frame.f_back ...: name = caller.f_code.co_name ...: finally: ...: del frame ...: del caller ...: ...: return name ...: ...: def func(): ...: print(current_function_name()) ...: ...: def another_func(): ...: print(current_function_name()) ...: ...: class A: ...: def method(self): ...: print(current_function_name()) ...: ...: f = func ...: ...: func() ...: another_func() ...: f() ...: ...: a = A() ...: a.method() func another_func func method With this we can only get the name of the definition so this might break for injected methods, but I guess for injected methods it would be difficult to manually pass the function name, anyways.

not sure. There are a few questions about the signature of the user functions (see https://github.com/pydata/xarray/pull/4896#issuecomment-779862647 and https://github.com/pydata/xarray/issues/3891#issuecomment-818377781), which I would like to answer before including this in a release (I might be wrong, but I think changing the signature after releasing is pretty hard)

Shall we merge?

sounds reasonable, but that would require a bigger change than just extending merge_attrs (which currently ignores encoding)

One thing to think about is .encoding.

IIUC we want to treat .encoding like .attrs eventually, so we need to design the signature of this callable to handle that especially since .encoding can contain "useful" attributes like "coordinates" and "bounds" after #2844.

One option would be ``` def my_combine_attrs(list_of_attrs_dicts, list_of_encoding_dicts): attrs = ... encoding = ... return attrs, encoding