Skip to content

Conversation

@jvmvik
Copy link
Contributor

@jvmvik jvmvik commented Jun 20, 2022

call to review new implementation, documentation, examples and more

@ilyazub ilyazub mentioned this pull request Sep 30, 2022
Copy link
Contributor

@ilyazub ilyazub left a comment

Choose a reason for hiding this comment

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

Thanks, @jvmvik!

We with @dimitryzub reviewed this PR and made some fixes that we've placed in a separate PR to this one: #2

decoder + ", available: json, html, object")


class Client(HttpClient):
Copy link
Contributor

Choose a reason for hiding this comment

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

Client extends HttpClient is a bit confusing.

README.md.erb Outdated
Comment on lines 29 to 62
First things first, import the serpapi module:

```python
import serpapi
```
You'll need a client instance to make a search. This object handles all of the details of connection pooling and thread safety so that you don't have to:

```python
client = serpapi.Client()
```
To make a search using SerpApi.com:

```python
parameter = {
api_key: "secret_api_key", # from serpapi.com
engine: "google", # search engine
q: "coffee", # search topic
location: "Austin,TX" # location
}
results = searpapi.search(parameter)
```
Putting everything together.
```python
import serpapi

parameter = {
api_key: "secret_api_key", # from serpapi.com
engine: "google", # search engine
q: "coffee", # search topic
location: "Austin,TX" # location
}
results = searpapi.search(parameter)
print(results)
```
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
First things first, import the serpapi module:
```python
import serpapi
```
You'll need a client instance to make a search. This object handles all of the details of connection pooling and thread safety so that you don't have to:
```python
client = serpapi.Client()
```
To make a search using SerpApi.com:
```python
parameter = {
api_key: "secret_api_key", # from serpapi.com
engine: "google", # search engine
q: "coffee", # search topic
location: "Austin,TX" # location
}
results = searpapi.search(parameter)
```
Putting everything together.
```python
import serpapi
parameter = {
api_key: "secret_api_key", # from serpapi.com
engine: "google", # search engine
q: "coffee", # search topic
location: "Austin,TX" # location
}
results = searpapi.search(parameter)
print(results)
```
The following example runs a search for `"coffee"` using your secret API key which you can find at [SerpApi Dashboard](https://serpapi.com/manage-api-key) page. The `searpapi.search` object handles all of the details of connection pooling and thread safety so that you don't have to:
```python
import serpapi
parameter = {
api_key: "secret_api_key", # from serpapi.com
engine: "google", # search engine
q: "coffee", # search topic
location: "Austin,TX" # location
}
results = searpapi.search(parameter)
print(results)

README.md.erb Outdated
Optionally, rhe HTTP connection can be tuned:
- timeout : connection timeout by default 60s
- retries : attempt to reconnect if the connection failed by default: False.
serpapi is reliable at 99.99% but your company network might not be as stable.
Copy link
Contributor

Choose a reason for hiding this comment

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

but your company network might not be as stable

Sounds too self-confident 🤣

README.md.erb Outdated

### Advanced settings
SerpApi Client uses urllib3 under the hood.
Optionally, rhe HTTP connection can be tuned:
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
Optionally, rhe HTTP connection can be tuned:
Optionally, the HTTP connection can be tuned:

README.md.erb Outdated
```

### Advanced settings
SerpApi Client uses urllib3 under the hood.
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
SerpApi Client uses urllib3 under the hood.
SerpApi Client uses `urllib3` under the hood.

README.md.erb Outdated
[![SerpApi-Python](https://github.com/serpapi/serpapi-python/actions/workflows/ci.yml/badge.svg)](https://github.com/serpapi/serpapi-python/actions/workflows/ci.yml)

[SerpApi]](https://serpapi.com) allows to scrape any search engine results.
It's easy, fast, easy, feature rich, cost effective, scalable and reliable.
Copy link
Contributor

@ilyazub ilyazub Sep 29, 2022

Choose a reason for hiding this comment

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

Sounds too self-confident :-)

Suggested change
It's easy, fast, easy, feature rich, cost effective, scalable and reliable.
It's easy, fast, feature rich API.

Comment on lines 24 to 34
if 'timeout' in parameter:
self.timeout = parameter['timeout']
else:
# 60s default
self.timeout = 60.0

# no HTTP retry
if 'retries' in parameter:
self.retries = parameter['retries']
else:
self.retries = False
Copy link
Contributor

@ilyazub ilyazub Sep 30, 2022

Choose a reason for hiding this comment

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

There are 3 required fields to the serpapi.Client constructor: timeout, retries, and api_key.

Others are passed to the search or other methods.

Here's what seems more convenient to me:

import serpapi

client = serpapi.Client(
  retries = 5, # or urllib3.util.Retry(connect=5, read=2)
  timeout = 4.2,
  api_key = "secret_api_key", # from serpapi.com
)

parameters = {
  "q": "coffee",
}

results = client.search(parameters)

print(results)

Comment on lines +67 to +69
# merge parameter defaults and overrides
fields = self.parameter.copy()
fields.update(parameter)
Copy link
Contributor

@ilyazub ilyazub Sep 30, 2022

Choose a reason for hiding this comment

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

This class hierarchy is very confusing while reviewing. We with @dimitryzub had to jump up and down to find the flow of parameters from constructors, and search to start method.

Comment on lines 83 to 97
if response.status != 200:
try:
raw = response.data.decode('utf-8')
payload = json.loads(raw)
raise SerpApiException(payload['error'])
except Exception as ex:
raise SerpApiException(raw) from ex

# HTTP success 200
payload = response.data.decode('utf-8')

# successful response decoding
if decoder == 'json':
return json.loads(payload)

Copy link
Contributor

Choose a reason for hiding this comment

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

response.data.decode('utf-8') and json.loads(payload) is called twice. So, our library slows down developer experience.

@jvmvik jvmvik merged commit d4a2aed into master May 8, 2023
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