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

fix #1 Comply with pylint #3

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Microturbine
Copy link

fix #1
Added timeout handling for requests.get.
And fixed error messages when ValueError occurs.

Added timeout handling for requests.get.
@vdmkenny vdmkenny self-requested a review May 27, 2024 20:20
Copy link
Owner

@vdmkenny vdmkenny left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! It looks fine to me except for the extra comments of which I'm not a fan.
It gives me some ideas to handle more exceptions, too.

cine-aalst.py Outdated Show resolved Hide resolved
cine-aalst.py Outdated Show resolved Hide resolved
@vdmkenny
Copy link
Owner

vdmkenny commented May 27, 2024

Actually, you're wrapping the try around quite a bit of code.
In my opinion, you should keep the try-except block around the specific lines of code you want to catch errors in, so like:

def get_movies_and_schedules():
    url = "https://cine-aalst.be/nl/movies/today"
    timeout = 10
    try:
        response = requests.get(url, timeout=timeout)
    except requests.exceptions.Timeout:
        print("Request timed out. Please check the internet connection.")
        sys.exit(1)
    except requests.exceptions.RequestException as e:
        print(f"Request failed: {e}")
        sys.exit(1)

    content = response.text
    pattern = r'<script id="storeData">window.storeData =(.+?);<\/script>'
    match = re.search(pattern, content)
    ...

It improves the readability, at least to me.
I used sys.exit(1) to quit the program early instead of returning the empty array.
There's no point in the program continuing after a fatal error like this.

I have seen the revised text. Approved.

Co-authored-by: Kenny Van de Maele <[email protected]>
Copy link
Author

@Microturbine Microturbine left a comment

Choose a reason for hiding this comment

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

I saw it. It looks fine.

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.

Comply with pylint
2 participants