Moved Tabulator Subclasses & Raise Errors for Static Dialect #25
Conversation
- Updated imports to new module locations in canada plugin. - Added extra checks to static dialects to raise errors if not matching.
- Renamed canada script.
ckanext/xloader/loader.py
Outdated
| try: | ||
| from ckanext.canada.tabulate import ( | ||
| CanadaStream as TabulatorStream, | ||
| CanadaCSVParser as TabulatorCSVParser |
There was a problem hiding this comment.
This is a little magical. Why not have these classes in our xloader fork if this is the only way they're used?
There was a problem hiding this comment.
Similar comment here, if xloader had an interface to let us provide stream and csvparser that would be cleaner. Importing from a specific theme from a reusable code base just feels like the wrong direction.
There was a problem hiding this comment.
@wardi sorry, they were in our Xloader fork. But will be needing to use them in Goodtables and Validation
There was a problem hiding this comment.
Yeah, so if you give xloader, goodtables and validation interfaces that we could call during configuration of ckanext-canada to pass in the classes and config then we don't end up with our forks of these depending on our specific theme plugin in a fragile way.
This is another argument for building validation features into xloader to reduce the complexity, but maybe there are some users that need one without the other?
There was a problem hiding this comment.
@wardi okay so just something like ITabulator and have methods like get_dialect, get_stream_class, get_parsers, and get_encoding??
Would all three plugins (goodtables, Xloader, Validation) need to have this plugin implementation in their codebase? So just copy the ITabulator class to each one?
There was a problem hiding this comment.
It could be something as simple as a function you can call to change the default configuration/register the new classes required in goodtables, xloader and validation. Doesn't have to be the same on all three.
There was a problem hiding this comment.
@wardi open-data/ckanext-validation@68e6e49 something like this??
There was a problem hiding this comment.
Or am I reaaaaly beneath monkey patching Tabulator classes in our Canada plugin hahaha
There was a problem hiding this comment.
that's an option, and one I've reached for in the past. Not a great option because monkey patching is super confusing for the person debugging a few years from now, but if it's extremely painful to implement an interface to allow validation to be configured I could understand. Just make sure to use lots of FIXME comments in ckanext-canada explaining why it was done that way :-/
- Only enforce dialect if the stream has guessed any.
- Created a tabulator interface.
- Move everything into canada plugin.
- Moved code to canada plugin.
feat(dev): module imports, raise errors;