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.

Sign up for GitHub

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

Jump to bottom

PIP optional dependencies #1771

Merged
merged 3 commits into from
Dec 13, 2023
Merged

PIP optional dependencies #1771

merged 3 commits into from
Dec 13, 2023

Conversation

JuliaLWang8
Copy link
Contributor

Implemented extra dependencies as discussed in #1766.

This can be tested locally via the command pip install -e .[extras] by replacing extras with:

  • nospam: requests dependencies for custom requests and testing
    • requests_cache, requests_ratelimiter
  • repair: for price repairing functionality
    • scipy

Maybe also add to #1084 that nospam is required to run tests and link the Installation instructions in the readme.

@JuliaLWang8
Copy link
Contributor Author

Regarding the naming, I think nospam makes sense concerning development because it's only needed for testing. However, since users could also install via the pip cmd to make custom requests (as in the readme), should we change it to reflect its use case?

@JuliaLWang8 JuliaLWang8 changed the title Added extra dependencies Feat/adding extra dependencies Dec 10, 2023
@ValueRaider
Copy link
Collaborator

should we change it to reflect its use case?

Isn't the use case same? Reduce spam on Yahoo.

@JuliaLWang8
Copy link
Contributor Author

should we change it to reflect its use case?

Isn't the use case same? Reduce spam on Yahoo.

The way I interpreted the readme 'Smarter scraping' section was that the use case of requests_cache is a custom way to make requests via modifying User-agent. I guess there's also caching and the uses of requests_ratelimiter include the rate-limiting to avoid triggering Yahoo's blocker, so nospam makes sense.

@ValueRaider
Copy link
Collaborator

The way I interpreted the readme 'Smarter scraping' section was that the use case of requests_cache is a custom way to make requests via modifying User-agent.

That's a README problem so my fault. To keep it concise I combined 2 examples:

  • custom User-agent on a session
  • replace requests with requests-cache

Maybe User-agent example should be removed.

@JuliaLWang8
Copy link
Contributor Author

Maybe User-agent example should be removed.

Should we change/remove it in the readme for this PR or just keep as is for now?

@ValueRaider
Copy link
Collaborator

Can lower scipy minimum to 1.6.3.

Should we change/remove it in the readme for this PR or just keep as is for now?

I don't mind if in this PR. As you had confusion, probably you're best to decide whether/how to change.

@JuliaLWang8
Copy link
Contributor Author

Can lower scipy minimum to 1.6.3.
I don't mind if in this PR. As you had confusion, probably you're best to decide whether/how to change.

Added these changes in my latest commit.

@ValueRaider
Copy link
Collaborator

I've made some 'personal preference' tweaks to README. If no complaint, then I'll merge.

@JuliaLWang8
Copy link
Contributor Author

I've made some 'personal preference' tweaks to README. If no complaint, then I'll merge.

Looks good to me!

@ValueRaider ValueRaider merged commit f32097e into ranaroussi:dev Dec 13, 2023
1 check passed
@ValueRaider ValueRaider mentioned this pull request Dec 23, 2023
Closed
@ValueRaider ValueRaider mentioned this pull request Jan 6, 2024
Merged
@ValueRaider ValueRaider changed the title Feat/adding extra dependencies PIP optional dependencies Jan 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JuliaLWang8 @ValueRaider