cleanup and adaptive use #5
Conversation
There was a problem hiding this comment.
Hi Christian,
Thanks for your interest in the project and for taking the time to contribute. I hadn’t really imagined that others would want contribute, hence the lack of contribution policy, development branch, automated testing and building, etc. So bear with me...
Firstly, good catch on the setup.py not packaging the data folder up. I wasn't familiar with MANIFEST.in, but it appears to be a fine choice to solve this issue as unfortunately I don't see a way for setuptools to just default to include everything under source control (would be my ideal solution for this simple project). I've added comments in-line with my remaining thoughts, please have a look.
For the extensions, I think it will be best if we create feature requests and work from there. Once this PR goes through I'll at least add a dev branch, contribution guidelines, a feature request template, and perhaps look into CI so that things are smoother going forward.
Thanks again!
Farhad
|
Ok, PR should be good to go now! |
|
I also went ahead and added a minimal Dockerfile for this as well for a personal project. Open to adding some more tooling like that? |
|
Thanks @cdbattags! I see you removed the "interim" statements from filing.py...You had mentioned that there were some examples of companies that you came across that this library didn't work for - were you able to find those? And yes, docker tooling is a nice idea. I guess you mean that you just added it locally though, right? Because I don't see it added here. Either way, we can update the README for that too to offer an alternative way of installing using just docker build && docker run. |
| # e.g. shares in Thousands, $ in Millions | ||
| unit_text = info_list[1] | ||
| unit_text = '' |
There was a problem hiding this comment.
@cdbattags just revisiting this PR...this one i'm not sure about. if you can find a case that doesn't have length 2 then i'd like to see it, otherwise i would actually prefer the runtime error here so that we're alerted to such cases.
There was a problem hiding this comment.
DBX 2019 (don't remember which statement), it doesn't provide a unit, hence number are good as they are.
Thank you for all the work you did.
Hi @farhadab,
Just wanted to go ahead and get this work-in-progress PR up.
Lemme know if you have any thoughts. I'd like to extend this primarily for EPS data.