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

refactor md functions into utils #67

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

refactor md functions into utils #67

wants to merge 5 commits into from

Conversation

wizzdom
Copy link
Member

@wizzdom wizzdom commented Jan 16, 2025

No description provided.

@wizzdom wizzdom requested a review from novanai January 16, 2025 15:12
@wizzdom wizzdom marked this pull request as ready for review January 16, 2025 21:37
src/utils.py Outdated
async with aiohttp_client.get(request_url) as response:
if response.status != 200:
raise Exception(
f"Failed to fetch the minutes. Status code: `{response.status}`"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather the aiohttp exception be raised with response.raise_for_status(). Then the specific aiohttp.ClientResponseError can be used in the except block instead of the generic Exception.

if "https://md.redbrick.dcu.ie" not in url:
try:
content = await get_md_content(url, aiohttp_client)
except Exception as e:
Copy link
Contributor

@novanai novanai Jan 16, 2025

Choose a reason for hiding this comment

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

Use aiohttp.ClientResponseError here for an error with the request, and add another except block for ValueError when the URL is invalid.

Or you could leave the URL validation here, and have get_md_content be purely be for making the request.

new_agenda_url = response.url
try:
new_agenda_url = await post_new_md_content(modified_content, aiohttp_client)
except Exception as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as earlier comment, but only need to catch aiohttp.ClientResponseError.

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