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

Add contrib package for MSSQL #1650

Merged
merged 3 commits into from
Apr 21, 2016
Merged

Add contrib package for MSSQL #1650

merged 3 commits into from
Apr 21, 2016

Conversation

traviscook21
Copy link
Contributor

This is primarily based off of the mysqldb.py module, but uses the _mssql library

This is primarily based off of the mysqldb.py module, but uses the _mssql library instead
@erikbern
Copy link
Contributor

looks good!

@Tarrasch
Copy link
Contributor

Cool. Some thoughts:


Can you add some more docs?

  • Is it read/write only or both?
  • Perhaps a minimal example as module level documentation.

Can tests be added?


If you mostly copy-pasted some other module, did you consider generalizing/inheriiting and stuff??


Note, all these things are optional. This can be merged as-is too since it's under contrib/.

@traviscook21
Copy link
Contributor Author

Hm, I think generalizing/inhereiting stuff is a good suggestion, but would probably require larger changes.

Generally, the code used for most of the methods is mostly identical to mysqldbpy and postgres.py.

One thought would be to add a abstract rdbms target class to rdbms.py. Then refactor the postgres, mysql, and mssql libraries to leverage that abstract class. I'm not sure if that's a change that y'all have discussed before/would be open to making.

I'm still thinking about how to make generalizable Task classes for the mssql library. The postgres library has CopyToTable and Query, which aren't use cases that I'm working with, but I'm thinking about a RunProcedure task class. I'm fairly new to luigi, so I'm still working on having the whole library cement itself in my mind.

Do you have any thoughts on the above, considering your much larger familiarity (duh) with the project?

@Tarrasch
Copy link
Contributor

Tarrasch commented Apr 15, 2016

Well. One decent thing to do in these situations when one doesn't have time to dive into the code base and refactor and stuff is to at least add some docs saying """This module is mostly a copy-paste of other_module.py. In the future one common interface could be derived.""". In addition to that I still think you at least should have an usage example in the module-level docs.

Other than that. We can merge this.

@traviscook21
Copy link
Contributor Author

@Tarrasch - Added the comments suggested above. This can be merged.

@Tarrasch Tarrasch merged commit 1f402aa into spotify:master Apr 21, 2016
This was referenced Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants