FormatAgent is irrelevant

[bblay]

FormatAgent is an irrelevant class and should be removed.

[esc24]

Please elaborate.

[bblay]

It's an object-oriented design term from one of the basic heuristics, "eliminate irrelevant classes".

[esc24]

I'm just trying to be more inclusive. Firstly, a simple statement claiming that it adds no beneficial behaviour/functionality helps everyone know what you're talking about. I'm all for being concise, but you can take it too far. While we're at it, referring to it as iris.io.format_picker.FormatAgent helps those less familiar with the codebase. Finally, a statement such as this is not really enough - please justify/elaborate on this claim. I'm not convinced it's true given the functionality in get_spec, but I'm happy to be convinced...

[bblay]

Whilst being concerned about over-heating the discussion, here are my impartial responses, in order:

• I think it is a clear, simple statement.
• This ticket is for people who are familiar with, or can navigate, the code.
• I can't teach OO in a github ticket, but there is no need/benefit for persisting _format_specs in an object.
• The functionality in get_spec does not warrant a class.
• We don't need to convince ourselves it's an irrelevant class, we need to justify it's existence, or remove it.

Perhaps we should move this to the discussion forum, but it seems a little trivial for that?

[rhattersley]

The Iris issues are a shared resource, not a personal memory jogger. So issues should be written for consumption by any developer (or even user). Clearly you've already thought about this issue, so please take the small amount of time to describe the problem and perhaps even the solution if you have one in mind. You never know, once you've done that someone else might make the code change for you! :wink:

[bblay]

I really don't know how to be any simpler or clearer. The problem: The class is irrelevant (an object-oriented design problem). The solution: Remove the class (or justify it's existence).

This is going nowhere so I'll just submit a PR when I can.

[rhattersley]

Hang in there! If you can face it, please don't give up on this issue just yet.

I really don't know how to be any simpler or clearer.

Could you give a quick code-sketch of the resulting API?

i.e. Instead of: handling_format_spec = iris.fileformats.FORMAT_AGENT.get_spec(os.path.basename(fn), fh) one would do ... ?

[bblay]

OK, thanks!

Instead of: handling_format_spec = iris.fileformats.FORMAT_AGENT.get_spec(os.path.basename(fn), fh)

We would do: handling_format_spec = iris.fileformats.get_spec(os.path.basename(fn), fh)

[esc24]

I believe you are suggesting moving the functionality out of a class and into a module. Presumably the associated data (the collection of registered format specs) needs to move with it, along with the means to add more specs. I like the idea of simplifying the namespace hierarchy (it took me a while to find FormatAgent :blush:), but I need convincing that putting the data and functionality at the module level rather than in a pseudo-singleton is the right way to go. FWIW I'm not wedded to the current approach - I'm only now understanding how it all works as a result of you raising this issue.

[bblay]

Are there any arguments in favour of having a class? Perhaps I can try:

• It might be tempting to gather the list, the add and the get into a class but these things actually make more sense at the module level. fileformats.add_format() is intuitive.
• It might be tempting to introduce a singleton but the module is a singleton.
• It has been suggested the get_spec method might be a reason to keep the class. I don't know why though. This should be examined further as there may be a good reason to keep the class?

Against having a class:

• The code would be easier to navigate (and therefore maintain/debug).
• There would be less classes in the system.
• Code that uses it would be more intuitive to read.

So there is no reasonable alternative, other than to put this stuff at the module level, is there?

[pelson]

Are there any arguments in favour of having a class?

The big one for me: iris.io.format_picker does not import iris. It is a wholly standalone piece of functionality which could happily be taken out of iris and deployed as a package in its own right.

If you make the fileformats module hold a global list of file formats it is entirely equivalent to a format agent instance living in the module, except with a FormatAgent at least you get methods (rather than module functions) and a pretty print capability that a list wouldn't...

For the record:

$> python
>>> import iris
>>> print iris.fileformats.FORMAT_AGENT
 * NetCDF OPeNDAP (priority 6)
 * UM Post Processing file (PP) (priority 5)
 * GRIB (priority 5)
 * NetCDF (priority 5)
 * NetCDF 64 bit offset format (priority 5)
 * NetCDF_v4 (priority 5)
 * NAME III (priority 5)
 * UM Fieldsfile (FF) post v5.2 (priority 4)
 * UM Post Processing file (PP) little-endian (priority 3)
 * UM Fieldsfile (FF) pre v3.1 (priority 3)
 * UM Fieldsfile (FF) ancillary (priority 3)
 * UM Fieldsfile (FF) converted with ieee to 32 bit (priority 3)
 * NIMROD (priority 3)

Don't get me wrong, I'm in favour of simplifying the format picking functionality, but I don't necessarily think nuking the FormatAgent is going to provide that simplification...

[bblay]

@pelson I don't suggest we change _"...iris.io.formatpicker does not import iris." Nor that we "...make the fileformats module hold a global list of file formats...". Just that we remove the FormatAgent class.

The argument about the pretty print capability seems reasonable but it still doesn't give the class a reason to persist any instance data, therefore it's just a bunch of functions, therefore it's not a valid class. We can get a pretty print in other ways.

[esc24]

If you are removing the class, where are you intending to store the list of registered format specs if not in the module?

FWIW I see data (the format specs) and functionality (registration of new and retrieval of matching specs) that should be encapsulated in an object. I think you are proposing to use the module object for the benefit of fewer classes and the singleton functionality. You can do this with a module and I won't lose much sleep over it, but I'd use a class. You have the ability to inherit from them, re-use them in relative isolation and instantiate multiple instances etc. The boundary and responsibility of a class should be well-defined. I'd argue this boundary is less clear with a module (although it doesn't have to be) and you risk blurring the responsibility as the module grows. As for the singleton argument, right now I don't need multiple format agents but it's not too hard to imagine a system where you have a default set and a custom set and you want to swap between them. This is difficult to implement with modules. Do we want to enforce a singleton? Probably not.

As I said, I won't lose much sleep over this but I don't agree that you will make the code easier to navigate by having a collection of methods and variables in a module rather than a class. We disagree on this - it doesn't mean either of us is right or wrong, it's just a difference of opinion.

[bblay]

if not in the module?

As I said, at the module level because fileformats.add_format() is intuitive.

it's not too hard to imagine a system where you have a default set and a custom set and you want to swap between them

That's when you need a class.

One final heuristic (remember, we should stand on the shoulders of OO giants): Be wary of classes with "manager" in the name. I'm pretty sure that cover this class.

[pelson]

Be wary of classes with "manager" in the name.

Emphasis is my own.

@bblay - I learn a lot from these OO discussions, but I don't feel like an iris github issue is the place to do it. To quote @rhattersley I think issues should be treated as "a shared resource, not a personal memory jogger" and that the best place for these things is on the iris-dev group along with a reasonable analysis of the problem and the benefits that an alternative would bring (i.e. "FormatAgent is an irrelevant class and should be removed." is subjective and clearly requires a degree of debate, for which iris-dev is ideal).

On that basis, I'm going to close the issue, though I do hope that that doesn't discourage anybody with an opinion joining in on the debate over at iris-dev.

[bblay]

no problem

[rhattersley]

Closing as discussed. Hopefully we'll see some action over on the discussion group...

[bblay]

https://groups.google.com/forum/#!topic/scitools-iris-dev/QMVwWJ8noX8