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

search function implemented. #3

Merged
merged 10 commits into from
Feb 18, 2024

Conversation

strobba
Copy link
Collaborator

@strobba strobba commented Feb 8, 2024

I felt a need for a method to search scb data from python, hence I implemented a search function. However, there is a request limit at scb which forced me to slow down the search. So by now its kind of slow. I raised an issue, with a potential solution to this. For now, the search can be slow imo.

@strobba strobba requested review from mgcth and docNord February 8, 2024 13:18
@strobba strobba self-assigned this Feb 8, 2024
Copy link
Contributor

@docNord docNord left a comment

Choose a reason for hiding this comment

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

Nice work!
The docstring is no longer correct as the function is no longer slow with the stored index, should be updated.
( there are tests which haven't been fulfilled yet)

@strobba
Copy link
Collaborator Author

strobba commented Feb 10, 2024

Nice work! The docstring is no longer correct as the function is no longer slow with the stored index, should be updated. ( there are tests which haven't been fulfilled yet)

Doc string removed. I'll fix linting. However, @mgcth why didn't my local ruff catch these things? Edit: Aha, ruff has no type checker. It is mypy who check the types...

@strobba
Copy link
Collaborator Author

strobba commented Feb 16, 2024

@mgcth considering the gist badge that fails in the test, where do I find it and how do i correct it?

@docNord
Copy link
Contributor

docNord commented Feb 18, 2024

@mgcth considering the gist badge that fails in the test, where do I find it and how do i correct it?

@mgcth I am interested in this answer too :)

@mgcth
Copy link
Contributor

mgcth commented Feb 18, 2024

You find it here #4 and needed to rebase/merge main into here.

@docNord
Copy link
Contributor

docNord commented Feb 18, 2024

Ah, so it was the same issue again. Thanks @mgcth 🙌😊

@strobba strobba merged commit 21285b7 into integrate-and-structure-current-code Feb 18, 2024
15 checks passed
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