I think I like the general idea of this (I believe I may have even suggested it), but I'm not happy with the implementation details. I'm sorry I don't have anything more specific to say at this time, but I have to give it more thought.

I think based on your explanation I'd want to see the end result of "phase 1" + "phase 2"

@embray - I'll just describe the plan I had rather than implement it all, so we can discuss it. The idea was that one format (for a specific class, such as Table) would correspond to one class, so for example FITSTableIO. This would then define for example (but not always all of) a reader, a writer, and an identifier method:

class FITSTableIO(...):

    def identify(...):
        ...

    def read(...):
        ...

    def write(...):
        ...

in the registry, we'd then replace the separate calls to register_* with:

register_io_class('fits', Table, 'astropy.io.fits.connect.FITSTableIO')

Now we could also consider changing this a little so that every sub-package that defines at least one IO reader/writer class also defines a 'hook' function that does all the calls to register_io_class. Then in registry we would only need a single call for a sub-package, even if it contains many IO classes, though we'd still have to list the format names explicitly, e.g.:

register_hook(['ascii', 'ascii.ipac', 'ascii.csv', ...], 'astropy.io.ascii.connect.hook')

One other thing that was planned is that when calling Table.read? one would be presented with a list of formats (I think this already works), and @taldcroft's idea was that to get help on a specific format one would do Table.read(format='fits', help=True). In a few words, the motivation for this is that one could not concatenate the docstrings from all formats into Table.read? because (a) it would be really long, and (b) one would need to import all the IO classes to get the content of the docstring. So then having Table.read(format='fits', help=True) dynamically loads the help for that format on-the-fly.

Yeah, that mostly sounds good. I think also specifying Table via its class may not be necessary either but I haven't investigated. Don't get me wrong, this is all in a good direction and I think my previous comment was unnecessarily negative; I was mostly just bothered by some of the details. I also don't have a lot of time/energy to spend looking this over right now, unfortunately.

@embray @taldcroft - I've been working on this more and one alternative (based on @embray's comments) is to basically simply list the formats in the registry and for each of them to specify the sub-package to import, e.g.:

BUILTIN_FORMATS = {
    'fits': 'astropy.io.fits',
    'votable': 'astropy.io.votable'
}

In addition we move the calls to register_reader etc back inside the format-specific sub-packages. If one of the formats is needed, the specified sub-package is imported and its readers/writers/etc will be registered. One issue however with this is that we can't know a-priori which formats apply to which classes and also which formats have readers/writers as opposed to say just readers. So if someone calls NDData.write(..., format='votable') we don't want to be importing astropy.io.votable because that format only applies to Table. So we can then do e.g.:

BUILTIN_FORMATS = {
    ('fits', NDData): 'astropy.io.fits',
    ('fits', Table): 'astropy.io.fits',
    ('votable', Table): 'astropy.io.votable'
}

We still don't know which formats are just readers and which are readers/writers though, so we could consider adding this information too. At this point we've duplicated a lot of information in the above structure and in the sub-packages (in the register* calls). So this may argue against the idea of having of leaving the sub-packages to register readers/writers because all that information has to be duplicated in the registry anyway.

So one solution is instead to go with something like we have in this PR, but tidied up a bit, so that the list of formats/functions is listed at the top in a module-level constant. But the readers/writers are still registered directly in registry.py so that we can avoid duplication of information. Does this sound reasonable?

@embray @taldcroft - do you have any thoughts on my comments above?

@astrofrog @embray - what about a hybrid approach where we keep all the self-registration within the sub-packages, but include some additional infrastructure that can auto-generate the appropriate code for astropy.io.registry? In this way there will only ever be one master source, but if you add/change things that would affect the registry to you need to a setup like python setup.py make_registry. This would import all the sub-packages and create the registry code, potentially complete with keyword docs and the whole kaboodle.

It wouldn't necessarily need to create code, since all the info could be encoded in a JSON file for instance. What would be nice is a data structure for each read or write format that has the necessary method names etc, but also has all available format-specific keyword options along with docs.

@taldcroft - this is an interesting idea. So would this be generated at build/install time? It is related to my suggestion in #3159 that we may want to consider the ability to have generic pre-processing steps for packages that would get run first before even any extensions are built. I can start up a discussion about this on the mailing list.

I don't know that such a thing has to be "generated"--can't it just be a hard-coded file that lives in the source tree?

@embray - what @taldcroft is suggesting is that all the readers, parameters, documentation would live in their own sub-packages, but then we'd have that information (at least which readers/writers are available, and parameters for readers/writers) available in the registry. However, for it to be useful it would have to duplicate information that exists in the sub-packages, so we are suggesting that it would be generated to not have to maintain two copies of the same information (if we don't do that, it will be a nightmare to sync).

@embray - certainly for io.ascii it is more convenient to generate than to hard code. By "generating" you can have the relevant information like format name and description defined locally in the actual class that defines the format. Moreover you can programmatically generate the list of available keyword options which rely on class inheritance to get fully defined. Finally, by maintaining the idea of registration via the base metaclass, it allows for user-defined formats that fully (and automatically) integrate with the I/O connection system.

@taldcroft - with these changes all the tests pass locally (let's see what Travis says). This includes a backward compatibility layer for the old register_reader/writer/identifier functions. But this should not be merged yet. In particular:

• Need to add deprecation warnings for the old functions
• I'm not sure if what I'm doing in io.ascii is optimal - in particular there may be a way of actually making the real format classes inherit from BaseIO rather than have wrapper classes that call the ui functions that then call the ASCII IO classes (seems inefficient). Maybe you could take a look at this @taldcroft?
• For now I removed the deprecated ASCII formats but can add them back if needed
• How do we want to deal with the make_registry.py script? Do we want to include it in astropy/io/registry/ and run it manually as needed for now?
• Add registry tests for new API (currently only being tested via backward-compatibility layer)

I'm sorry I haven't really had a chance to look this over yet, but just at a glance it looks like it's in a good direction. I'll get back to it soon as I can.

@embray @taldcroft - I think we are going to have to postpone this until 1.1 because I think there are other priorities to focus on by the end of the week. As this PR shows, we can maintain backward compatibility relatively easily, so it's not as crucial as I thought.

How would this deal with formats where the same format can be read as a Table, but also as NDData and maybe a Spectrum, e.g. a fits file? I'm sure it's in there, I just cannot find it.

I think that postponing this is reasonable.

@hamogu - as a user you differentiate with the class, i.e. Table.read('f.fits') or NDData.read('f.fits'). In the connector classes there is the _supported_class attribute. It would be nice to see this in action for a class other than Table. I guess this will come after NDData becomes a reality.

@taldcroft - oops I wanted to talk to you about this PR at the workshop... It would be great if you could try and review what is here currently at some point in the next few weeks and if it seems reaonable to you, I can continue to move ahead with it. The rebase is going to be bit a bit of a nightmare, so I will wait until you've had a chance to look over this (and will rebase if you are ok with it)

@taldcroft - do you have any initial thoughts on this PR? I would like to try and make sure it gets sufficient testing prior to 1.1 if we are going ahead with it.

Sorry, I'll have a look this week. It's been a little while...

@astrofrog - I got myself back up to speed on this and it looks great so far. I haven't dug into the nitty gritty but as far as the big concept I don't see any reason to not proceed.

The next thing that would be really nice is to get some code demonstrating the real user-facing benefit of this whole refactor, namely better docs. The first thing I would love to see is for Table.read and Table.write to have a dynamically generated list of available formats and extensions. The next is the help keyword to output the docstring for the reader/writer for the selected format.

Digging through old FITS PRs. Is this one still relevant? It has merge conflict.

@pllim - I still want to do this, but I'll likely need to start from scratch. I don't mind if we close it since I'll likely never finish this PR as-is.

We have an issue for this: #1046 - so this can be closed if others agree

I agree. 😄

Sometimes, you have to know when to give up on a pull request! 😆