24 common csv parser

[sgoldenCS]

This pull request should close #24 and close #33. The implementation of CSVParser is essentially the PandasParser with the file_format field forced to 'csv'.

Personally, I'm not too fond of this solution as it will require a different parser for every file format and will result in a large amount of code duplication. However, I don't see another way to address #24 and #33 since it's apparent the current solution is not acceptable....

[dlersch]

@sgoldenCS , this looks good. thank you. I have two suggestions (that I am happy to discuss):

1. Rename the utest_pandas_parser_v0.py to: utest_csv_parser.py just so that it is consistent with everything else in the repo and is consistent with the parser name
2. Remove the pandas_parser_v0.py You implemented a nice utility that does everything that the pandas_parser did. If people need a json_parser for example, they can just use your util and write their own parser, following your example

[sgoldenCS]

@dlersch I'm happy to rename the unit test. It's just a name anyway.

As for removing the pandas_parser, I'm hesitant to do that. While the utility I've implemented does the same work, I really question the idea of having multiple essentially duplicate parsers. I know @schr476 believes that it's too complicated, but if the goal of the modules is to reduce unnecessary work, I don't see how writing 150 lines of duplicate code for every file format makes any sense. The code is almost the same size as the full "pandas_parser"! It also fragments the code base and introduces more opportunities for bugs.

If we want a registered version for each file type, that could be solved by properly addressing #32 and using the pandas_parser. If we're unhappy with the "pandas_parser" name, we can agree on a more generic name like "base_parser_to_pandas". Ultimately, no good rationale for making these changes has been offered besides complaints about naming...

[dlersch]

@sgoldenCS I do see your points. If you could just rename the unit-test, that would be great and I will accept your pull-request. As for a more generic name for the pandas_parser -> How about dataframe_parser ? The object returned is technically a dataframe . This is just a suggestion. I have no attachments to the name

[schr476]

@dlersch I'm happy to rename the unit test. It's just a name anyway.

As for removing the pandas_parser, I'm hesitant to do that. While the utility I've implemented does the same work, I really question the idea of having multiple essentially duplicate parsers. I know @schr476 believes that it's too complicated, but if the goal of the modules is to reduce unnecessary work, I don't see how writing 150 lines of duplicate code for every file format makes any sense. The code is almost the same size as the full "pandas_parser"! It also fragments the code base and introduces more opportunities for bugs.

If we want a registered version for each file type, that could be solved by properly addressing #32 and using the pandas_parser. If we're unhappy with the "pandas_parser" name, we can agree on a more generic name like "base_parser_to_pandas". Ultimately, no good rationale for making these changes has been offered besides complaints about naming...

I think we agreeed that the goal of the parser is to read a particular data format.
We can make the default output format to be Pandas and converters to another format as needed.

[sgoldenCS]

I don't remember if we agreed to that, but I don't think any agreement invalidates my points. We can still have a registered version of the parser for each data format without having to rewrite all of the code each time we need a new file format.

[schr476]

@sgoldenCS , If we conform to one output format (Pandas) then we can write converters to satisfy other needed.
Therefore you write a parser per data type and converters as needed (which I would assume is relatively small)

[sgoldenCS]

I'm not sure I follow. I haven't mentioned output data types at all, so why are you talking about output converters?

I feel this needs to be an actual conversation, not just GitHub comments. I don't believe having these disagreements on a (semi-)public platform is fair to me or anyone else who receives notifications for it.

[schr476]

I'm not sure I follow. I haven't mentioned output data types at all, so why are you talking about output converters?

I feel this needs to be an actual conversation, not just GitHub comments. I don't believe having these disagreements on a (semi-)public platform is fair to me or anyone else who receives notifications for it.

Pandas is the output type for the CSVParser.
For simplicity, I propose we use Pandas DFs as a common output format and move forward.

[sgoldenCS]

I've removed the registration of the PandasParser and updated the name of the code file. I also removed all references to Pandas in the file that weren't required.

I will be preparing a small presentation to explain how I believe the "parser_to_dataframe.py" implementation will allow for much faster development while still maintaining good versioning practices.

@schr476 and I had a discussion this morning and I like the idea of defining a Parser Plugin to formalize the process of creating read_functions for the parser_to_dataframe.py module. If we agree on this approach, we can make an issue/branch and implement it.

[sgoldenCS]

After my conversation with @schr476 yesterday, I have updated a few things.

• The unit test has had naming issues fixed
• The unit test now prints all logging messages (debug and above) to stdout
• The parser_to_dataframe now accepts a "registry_config"
• Documentation has been added in the comments of parser_to_dataframe to explain the configuration priorities
  • Highest priority is anything in the "config" that should be used by the user
  • Next is anything defined in the "registry_config", which should be reserved for the registry to pass kwargs to the module
  • Any remaining unspecified arguments are set by a default config defined in the module
• The registry now registers the CSVParser using Parser2DataFrame with kwargs={'registry_config': {'file_format': 'csv'}}

The unit test for the CSVParser still works correctly and completes all tests, so none of these changes should break anything.

A few things to note:

1. The csv_parser_v0.py code has not been deleted but is no longer registered. Since the new registered CSVParser implements all of the same functionality, we could delete it if people agree.
2. The parser_to_dataframe.py code could use the new utility functions instead of the implementations in the module (they're identical, but using utilities could be nice)
3. Using the registry_config works for now, but we should agree on an approach (see Registry Kwargs Updating Issue #32)

[sgoldenCS]

We can complete this pull request and its associated issues. Any further changes can be made through a future issue/pull if we need to make them, but this code should serve as a good starting point.

I've deleted the csv_parser_v0.py file to avoid confusion as it is not registered and its functionality is fully implemented in Parser2DataFrame. The parser_utilities.py file in the utils folder is not being used, but the functions defined there could still be useful to someone in the future, so I'll leave them there for now.