New answers tagged web-scraping
0
Yes, right now this would be inefficient, I'd definitely say that using
a predictable filename would be the first key change here, if the link
target or description isn't usable as part of the filename, consider
hashing it. Simply enumerating all links is, well, suboptimal: What
if, even if previous files are kept around and not overwritten, between
runs ...
1
Threading
I'd like to join the others in recommending you drop your threading. You're also doing it wrong - you're only grabbing the semaphore for creating the thread, but you never acquire it inside of the thread when executing.
If you use a lock of semaphore, do it with a context manager:
with my_semaphore:
# code...
And if you can't, you're doing ...
2
Ignoring threading, it's usually best to benchmark your code on a single thread first, then move to multiple threads as necessary. Some glaring things that could yield slower performance:
You are not using a Session for requests. There is not a guarantee that sessions are thread-safe, but you could allocate a session for each thread, giving each thread it's ...
2
Whitespace formatting
Apply a linter that will give you PEP8 suggestions. Among other things, it will suggest the following:
import os,gc,queue
There should be spaces after those commas. It'll also suggest that there be one line per import.
class add_line:
should be
class AddLine:
and have a couple of newlines before it.
else :
shouldn't have a ...
0
Why have main() be a single line calling a single function? Either rename main() to get_xkcd() or just call get_xkcd().
Also, maybe use BeautifulSoup instead of lxml.
3
Constant reuse
Store your base URL, "https://www.example.com", in a constant for reuse.
HTTP codes
Some status codes, such as 200, are common and obvious, while others such as 429 are less so. Fundamentally these are all magic numbers, and requests.codes contains symbols - including too_many_requests.
URL construction
allposts = [f'https://...
8
Not a problem in terms of functionality (seems to have been covered pretty well already) but in terms of clarity/readability:
Misleading name get_penultimate
It appears to functionally be get_last (get_ultimate, if you will), so there is a clear mismatch between this name and the implemented functionality. The functionality makes sense, so the name appears ...
10
As mentioned in a comment by Thomas, another option is to use XKCD's JSON interface rather than scraping HTML:
import requests, time, tempfile, os.path
from shutil import copyfileobj
path = tempfile.mkdtemp()
print(path)
f_name = "{num:04d}-{safe_title}.{ext}"
current_comic = requests.get("https://xkcd.com/info.0.json").json()
# Iterates over numbers from ...
86
Stop hammering XKCD server
You basically wrote a denial-of-service attack, since you try to connect as fast and as often as possible to the server.
By sharing this code on CodeReview, it becomes a distributed denial-of-service.
Just in case it wasn't clear : don't do that.
I suppose the load of XKCD servers is high enough that your code won't make any ...
7
As for the code below, I'd appreciate any feedback, particularly in regards to threading
You might want to use a ThreadPoolExecutor to manage your threads. This approach has two advantages:
The executor can be used as a context manager to make sure all the threads are joined.
It lets you limit the number of threads in the thread pool.
Example:
with ...
10
for page in reversed(range(latest_comic - pages + 1, latest_comic + 1)):
print(f"Fetching page {page} out of {latest_comic}")
try:
url = get_images_from_page(f"{BASE_URL}{page}/")
thread = threading.Thread(target=save_image, args=(url, ))
thread.start()
except (ValueError, AttributeError, requests.exceptions.MissingSchema):...
14
Globals
As it is, there would be an advantage to LOGO being a local instead of a global - it's only used by show_logo, and moving it there would clean up the global namespace.
That said (and as others have pointed out), it's fairly common to see stuff like this at the top of Python files in global scope. However, the larger issue is that if you move it to ...
7
Just wanted to add that you may want to consider using Polly instead of rolling your own retry mechanism - this is a library built specifically for this kind of retry mechanism (as well as many more complicated scenarios). Using Polly would look something like the following.
Note that I have renamed the attempts parameter to retryAttempts to make it clear ...
6
After our initial comment-review-update sequence/loop there is not much left for a review but still, one more thing. You can get rid of the if entirely when you add a when filter to the excpetion:
catch (WaitTaskTimeoutException) when (attempts > 0)
{
//add some delay
await Task.Delay(10000);
return await ...
Top 50 recent answers are included
