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

Use new HTTP API to access the database #25

Merged
merged 26 commits into from
Dec 13, 2022
Merged

Use new HTTP API to access the database #25

merged 26 commits into from
Dec 13, 2022

Conversation

ASHuenchuleo
Copy link
Contributor

Description

Extended the client class to allow for direct querying of the database. Supported output formats are now json, votable, pandas and csv. Test cases were also added.

Related Issue or Feature

Closes #24

Motivation and Context

Direct access to the database was needed, so an HTTP API was developed and this client was updated to use the API.

How Has This Been Tested?

The new unit tests are in tests/test_direct.py. Integration testing with a database with the production schema, has not done yet.

database, and the test cases. Tests assume the client now accepts csv
replies, but this may be reversed if it's better to just modify the API
itself.
@ASHuenchuleo ASHuenchuleo linked an issue Nov 21, 2022 that may be closed by this pull request
@ASHuenchuleo ASHuenchuleo self-assigned this Nov 21, 2022
@ASHuenchuleo ASHuenchuleo marked this pull request as draft November 21, 2022 18:01
@ASHuenchuleo ASHuenchuleo marked this pull request as ready for review November 22, 2022 13:04
Copy link
Contributor

@pcastelln pcastelln left a comment

Choose a reason for hiding this comment

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

Dejé algunos comentarios específicos en los archivos.

En general me gustó harto que ahora se usen las distintas clases para la respuesta basado en el formato.

Mi mayor temor es en la inicialización de objetos tipo Alerce, porque no me queda claro que todas las opciones vayan a ser respetadas y no vi un test respecto a eso.

alerce/exceptions.py Outdated Show resolved Hide resolved
alerce/utils.py Outdated Show resolved Hide resolved
alerce/utils.py Outdated Show resolved Hide resolved
alerce/utils.py Outdated Show resolved Hide resolved
tests/test_search.py Outdated Show resolved Hide resolved
alerce/core.py Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@ASHuenchuleo
Copy link
Contributor Author

Files are now Black compliant

@codecov
Copy link

codecov bot commented Nov 25, 2022

Codecov Report

Merging #25 (0f4691a) into main (9ac3b31) will increase coverage by 4.11%.
The diff coverage is 98.92%.

❗ Current head 0f4691a differs from pull request most recent head 5bcd9ed. Consider uploading reports for the commit 5bcd9ed to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
+ Coverage   82.91%   87.02%   +4.11%     
==========================================
  Files           6        7       +1     
  Lines         316      370      +54     
==========================================
+ Hits          262      322      +60     
+ Misses         54       48       -6     
Impacted Files Coverage Δ
alerce/utils.py 96.93% <98.43%> (+2.70%) ⬆️
alerce/core.py 100.00% <100.00%> (+20.00%) ⬆️
alerce/direct.py 100.00% <100.00%> (ø)
alerce/exceptions.py 91.30% <100.00%> (+1.30%) ⬆️
alerce/search.py 97.82% <100.00%> (+7.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ac3b31...5bcd9ed. Read the comment docs.

Copy link
Contributor

@pcastelln pcastelln left a comment

Choose a reason for hiding this comment

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

Añadí algunos cambios menores directamente, pero el resto se ve muy bien.
Aprobado

@dirodriguezm dirodriguezm self-requested a review December 1, 2022 12:39
@ASHuenchuleo ASHuenchuleo merged commit 75a656c into main Dec 13, 2022
@ASHuenchuleo ASHuenchuleo deleted the feature/use-api branch December 13, 2022 13:05
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.

[Feature] Access the database via the HTTP API
4 participants