Skip to content

fix samair parser#34

Merged
pgaref merged 1 commit intopgaref:masterfrom
Hessu1337:master
Jul 6, 2017
Merged

fix samair parser#34
pgaref merged 1 commit intopgaref:masterfrom
Hessu1337:master

Conversation

@Hessu1337
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Owner

@pgaref pgaref left a comment

Choose a reason for hiding this comment

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

test_SemairProxyParser is failing since the mocked html is tailored to the previous provider format!

Need to fix that before merge!

self.logger.debug("Using proxy: {0}".format(str(self.current_proxy)))
request = requests.request(method, url, proxies={"http": self.current_proxy},
headers=headers, data=data, params=params, timeout=req_timeout)
headers=headers, data=data, params=params, timeout=req_timeout, allow_redirects=False)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

What is the reason behind disabling redirects?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The reason is to avoid requests.exceptions.TooManyRedirects raised by gathered proxy that redirect traffic to another website

Copy link
Copy Markdown
Owner

@pgaref pgaref Jul 5, 2017

Choose a reason for hiding this comment

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

Since proxy providers could use redirects in some cases, I would recommend using the maxRedirects property instead - a default value could be provided as method argument (i.e 3)

The only downside is that in this case we should also catch the TooManyRedirects Exception.

Let me know what you think

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ye, sounds good, maybe being able to pass allow_redirects from generate_proxied_request() would be cool too!

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Sure, do want to work on what we discussed above (maxRedicts, method arguments, and test_SemairProxyParser) or should I have the ticket reassigned?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I could do the method argument but not test_SemaireProxyParser since I don't understand the problem, sry my first contribute on github

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

In the tests/mocks.py file samair_mock content should be identical to what you see in the real web page. So when you run the test_providers all the tests should pass.

No worries at all, push your changes once you are ready and I will pick it up from there.

Cheers,
Panagiotis

# ports[key] = value

table = soup.find("table", attrs={"id": "proxylist"})
table = soup.find("div", attrs={"id": "proxylist"})
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Looks good

@pgaref pgaref merged commit e86d44a into pgaref:master Jul 6, 2017
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.

2 participants