Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduces Serializer/Parser interface members to only those items that are required by the Reader/Writer #671

Closed

Conversation

@christophano
Copy link

christophano commented Apr 20, 2017

Work in progress, opened for feedback - related to #596.

I have created a base interface for ICsvParser named IParser which contains only the members required by CsvReader. I have also introduced a type argument to CsvReader defining the type of IParser to be used. A default CsvReader class has been introduced which defined TParser to be an ICsvParser. It has the specialised constructors - the base class only has the core constructor.

I've also done the same for ICsvSerializer and CsvWriter.

The end result for users of the library should be fairly transparent. They can still use the CsvReader/CsvWriter classes with all of the same constructors, methods and properties.

I'm considering going a step further and segregating some of the configuration interfaces. This may be slightly more complicated as there tends to be a single class which implements several configuration interfaces.

Any thoughts or feedback?

@jamesbascle

This comment has been minimized.

Copy link
Contributor

jamesbascle commented Apr 20, 2017

@jamesbascle

This comment has been minimized.

Copy link
Contributor

jamesbascle commented Apr 20, 2017

@christophano

This comment has been minimized.

Copy link
Author

christophano commented Apr 20, 2017

Haha, yes the Excel project is mine, so these changes are outrageously selfish!
It would be useful for anyone else who wishes to implement their own adapters too though.

@christophano

This comment has been minimized.

Copy link
Author

christophano commented Apr 20, 2017

I've done a little bit of work to the configuration interfaces.
This does mean that the base CsvReader<,> and CsvWriter<,> classes (which are now abstract) now require 2 type arguments, but this shouldn't affect the end user, as they will still use the concrete CsvReader and CsvWriter classes, which should have the same surface as previously.

@christophano christophano force-pushed the christophano:interface-segregation branch from 109975e to 4c04b14 Apr 24, 2017
@christophano christophano force-pushed the christophano:interface-segregation branch from 4c04b14 to 4e28e74 May 5, 2017
@JoshClose

This comment has been minimized.

Copy link
Owner

JoshClose commented Aug 26, 2017

Lots of other changes happened that cause this not to be merged, and we discussed on the issue for this. If things need to be changed up, let me know soon, as I'll be releasing 3.0 soon now. Only a few issues left.

@JoshClose JoshClose closed this Aug 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.