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

Remove usage of PHP alias in the Python client #663

Merged
merged 20 commits into from Oct 12, 2023

Conversation

rzats
Copy link
Contributor

@rzats rzats commented Sep 22, 2023

Secondary PR to cmu-delphi/delphi-epidata#1288 that should be merged & deployed alongside it - fixed the delphi-epidata version to the newest client.

@capnrefsmmat
Copy link
Contributor

Since the Epidata cient API is changing, we'll need to update the version number required in setup.py along with these changes, so users are guaranteed to have the updated version of the client.

@rzats
Copy link
Contributor Author

rzats commented Sep 22, 2023

@capnrefsmmat good point! I bumped the version once and will change it again if the release ends up being e.g. 4.1.3.

Copy link
Contributor

@melange396 melange396 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, aside from the pending versioning discussion and any resulting version number changes. I would wait to merge til then.

@melange396
Copy link
Contributor

Did you get a chance to run tests for this with the updated client from cmu-delphi/delphi-epidata#1288 ? I think you might need to update the .return_values for the @patched async_epidata():

@patch("delphi_epidata.Epidata.async_epidata")
def test__async_fetch_epidata(mock_async_epidata):
mock_async_epidata.return_value = [({"message": "failed"}, {"time_values": 20200402})]

@melange396
Copy link
Contributor

can we update the changelog, and then follow this set of instructions to update the version number for the package and rebuild docs and other artifacts? then we will be prepped to upload to PyPI when everything else is done.

@melange396
Copy link
Contributor

@dshemetov makes a good point in cmu-delphi/delphi-epidata#1288 (comment) ... we should probably put documentation and show warning messages for deprecating _async_fetch_epidata(), just in case anyone is using it.

Comment on lines 7 to 9
- Remove usage of PHP alias in the Python client.
- Use Python package-specific "user agent" string.
- Mark :py:func:`_async_fetch_epidata` as deprecated.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Remove usage of PHP alias in the Python client.
- Use Python package-specific "user agent" string.
- Mark :py:func:`_async_fetch_epidata` as deprecated.
- New feature: Web requests now identify themselves with a `covidcast`-specific "user agent" string.
- Update: Now uses newer version of the `delphi-epidata` libarary.
- Bug fix: Removed usage of PHP alias endpoint for compatibility with new `delphi-epidata` version.
- Deprecated function: Private method :py:func:`_async_fetch_epidata` has been deprecated.

@melange396
Copy link
Contributor

@rzats this is failing at the linter now, i guess you didnt go through that part of the tests?

@melange396
Copy link
Contributor

linting/pydocstyle is happy now, but now python tests are failing... @rzats can you take a look?

@rzats
Copy link
Contributor Author

rzats commented Oct 12, 2023

@melange396 latest round of tests is passing!

@melange396 melange396 merged commit d2aa655 into main Oct 12, 2023
4 checks passed
@melange396 melange396 deleted the rzatserkovnyi/api-php-refactor branch October 12, 2023 18:25
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.

None yet

4 participants