Refactor isatab loader

[terazus]

@ptth222 first pass at the refactoring

[coveralls]

coverage: 81.281% (+0.07%) from 81.212%
when pulling e3c1912 on DomRefactorTabLoader
into d7aa027 on issue-511.

[ptth222]

So you are basically just turning most of the code that's there into classes?

I figured out what I was talking about in the meeting when I said that there were different loaders.

1. In convert/isatab2json.py if you use the old parser it uses the parser in io/isatab_parser.py.
2. In convert/isatab2json.py if you use the new parser it uses isatab.load(), but in isatab/validate/core.py it uses load_investigation(). Both of these end up using read_investigation_file() from isatab/load/read.py though. Validation works with the df_dicts version while the converter uses the object version that load() creates. I got confused because all the nesting functions say "load" or "read" so it seemed like they were independent.

[terazus]

So you are basically just turning most of the code that's there into classes?

Yes. I really dislike these nested functions and these huges block of codes. I've split the logic in dedicated methods and typed variables to 1. respect separation of concerns 2. make it easier to identify what is going on in code blocks.

I figured out what I was talking about in the meeting when I said that there were different loaders.
In convert/isatab2json.py if you use the old parser it uses the parser in io/isatab_parser.py.

You are right. The class here https://github.com/ISA-tools/isa-api/blob/master/isatools/convert/isatab2json.py#L65 should be deprecated. I will discuss with @proccaserra to see what we should do here. How about raising a DeprecationWarning ?

In convert/isatab2json.py if you use the new parser it uses isatab.load(), but in isatab/validate/core.py it uses load_investigation(). Both of these end up using read_investigation_file() from isatab/load/read.py though. Validation works with the df_dicts version while the converter uses the object version that load() creates. I got confused because all the nesting functions say "load" or "read" so it seemed like they were independent.

I agree that the naming is questionable. However the way it works make sense: the object creation through the load() function may 1. Crash or 2. Not build properly but be silent about it. This is why the validation is at the dataframe level.

[ptth222]

You are right. The class here https://github.com/ISA-tools/isa-api/blob/master/isatools/convert/isatab2json.py#L65 should be deprecated. I will discuss with @proccaserra to see what we should do here. How about raising a DeprecationWarning ?

That class is still being used by the function above it. I assumed it was essentially left in to be compatible with older versions. Having the new version as a parameter may be enough of an indication. A deprecation warning would make it more explicit though. Might also consider changing the "use_new_parser" default to True or change it to "use_old_parser", so that the default is not using deprecated things.

I agree that the naming is questionable. However the way it works make sense: the object creation through the load() function may 1. Crash or 2. Not build properly but be silent about it. This is why the validation is at the dataframe level.

I wasn't trying to suggest any of this needed changing. I was just explaining what was brought up and why I got confused. I think the way things are is appropriate. Maybe a name change or adding to the function documentation could make things clearer, but it's not completely necessary