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

Ruff Linter configuring #293

Closed
wants to merge 0 commits into from
Closed

Ruff Linter configuring #293

wants to merge 0 commits into from

Conversation

deedy5
Copy link
Contributor

@deedy5 deedy5 commented Apr 10, 2024

  1. Enable Ruff Linter rules (https://docs.astral.sh/ruff/linter/#rule-selection),
  2. Update requirements, delete autoflake, flake*,
  3. make lint: mypy automatically install known missing stub packages for third-party libraries,
  4. Fixe errors that Ruff Linter started to show with new rules.

@@ -69,11 +69,11 @@ def write_callback(ptr, size, nmemb, userdata):
callback = ffi.from_handle(userdata)
wrote = callback(ffi.buffer(ptr, nmemb)[:])
wrote = ensure_int(wrote)
if wrote == CURL_WRITEFUNC_PAUSE or wrote == CURL_WRITEFUNC_ERROR:
if wrote in (CURL_WRITEFUNC_PAUSE, CURL_WRITEFUNC_ERROR):
Copy link
Collaborator

@perklet perklet Apr 15, 2024

Choose a reason for hiding this comment

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

I know that in (...) is the preferred style, but this function is quite compute intensive, let's keep it the original form.

pyproject.toml Outdated
@@ -5,7 +5,7 @@ authors = [{ name = "Yifei Kong", email = "kong@yifei.me" }]
description = "libcurl ffi bindings for Python, with impersonation support."
license = { file = "LICENSE" }
dependencies = [
"cffi>=1.12.0",
"cffi>=1.16.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer to keep the dependency version as low as possible if there are no serious bugs or features only available in new versions. The cffi lib is such a common dependency for many projects, see here, raising it may cause conflicting situations for our users.

pyproject.toml Outdated
]


[build-system]
requires = ["wheel", "setuptools", "cffi>=1.12.0"]
requires = ["wheel", "setuptools", "cffi>=1.16.0"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here, too.

else:
return item
item_map = {"chrome": cls.chrome, "safari": cls.safari, "safari_ios": cls.safari_ios}
return item_map.get(item, item)
Copy link
Collaborator

@perklet perklet Apr 15, 2024

Choose a reason for hiding this comment

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

This function is also called quite frequently, I prefer to keep the if-else style instead of table-driven style. Perhaps, the match expression is equivlent to if-else, but that's only available in later versions.

if self.base_url:
url = urljoin(self.base_url, url)
c.setopt(CurlOpt.URL, url.encode())

# data/body/json
if isinstance(data, dict):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, if-else style if preferred for performance reasons.


# url
if self.params:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This has been updated #296, parameters are not dicts.

c.setopt(CurlOpt.CUSTOMREQUEST, method.encode())
if method == "POST":
Copy link
Collaborator

Choose a reason for hiding this comment

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

For POST, setting CUSTOMREQUEST can be problematic. A common scenario is that user submits a form via POST, and the server redirects to a page that you need to GET. CurlOpt.POST is actually made for that, while CUSTOMREQUEST will send two POST requests.

@@ -567,10 +567,9 @@ def test_stream_empty_body(server):
def test_stream_incomplete_read(server):
with requests.Session() as s:
url = str(server.url.copy_with(path="/incomplete_read"))
with pytest.raises(requests.RequestsError) as e:
Copy link
Collaborator

@perklet perklet Apr 15, 2024

Choose a reason for hiding this comment

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

The nested style is preferred to show the nested environments we are creating. It's not like we are opening two files here.

@@ -590,9 +589,8 @@ def test_stream_incomplete_read_without_close(server):
def test_stream_redirect_loop(server):
with requests.Session() as s:
url = str(server.url.copy_with(path="/redirect_loop"))
with pytest.raises(requests.RequestsError) as e:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

@deedy5
Copy link
Contributor Author

deedy5 commented Apr 16, 2024

accidentally closed

@deedy5 deedy5 mentioned this pull request Apr 16, 2024
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