Peer_review_feedback - Githubissues

UBC-MDS / pyntrinio

Python Software package development. Converts Intrinio objects (Historical financial market data) to native python and pandas objects.

MIT License

0 stars 3 forks source link

As per Braden's review from this thread, following changes were made

Addressed

Docstring description should explicitly mention the arguments - Corrected
- A small typo I picked up on: pytrinio instead of pyntrinio - Corrected
It might have been more clear if you also listed you have to install intrinio-sdk first - added note under "dependencies" to install intrinio-sdk first
You can specify python in the markdown to make the example code look nicer and more readable - Addressed
In the coverage add poetry install as a second step otherwise it won’t run -- Addressed with proper installation instructions updated in readme
Readthedocs does not seem to contain the functions -- Issue regarding this addressed to our TA Javier and to tiffany here
No mention of how to import - Addressed
Specify python in the markdown to make the example code look nicer and more readable -- Addressed

Will not address at this time

Perhaps have the function descriptions in the readme in a table with description, input, output to make it more readable
- Excellent suggestion. We shall implement this in the next release of the package when other feedback like make function names less verbose, add frequency arguments to functions etc. are implemented consistently across both Python (pyntrinio) and R (rintrinio)
Separate .py files for each function

As per Mike's review from this thread, following changes were made

Addressed

Missing instruction to install intrino-sdk before downloading pyntrinio - added note under "dependencies" to install intrinio-sdk first
Leave instruction on how to import the functions in the examples section of the readme - Addressed
example in the pyntrinio.py file for gather_financial_statement_time_series() doesn't work - Corrected

Will not address at this time

Incorrect error msg when a ticker not covered in the sandbox data for the US Fundamentals and Stock Prices data feed (ex. 'GOOG')
- We are aware of this issue, and the lack of consistency in the error msg displayed arises from the fact that our (student) access is limited to the free version of Intrinio (as mentioned in the readme). And we did not want to limit the access of the package to only free users
Additional note about this updated in the readme for the time being
In future iterations, all functions could be modified to work with both free and paid access to the Intrinio data

UBC-MDS / pyntrinio