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

Dev #16

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

Dev #16

wants to merge 6 commits into from

Conversation

milouk
Copy link
Owner

@milouk milouk commented Oct 22, 2024

No description provided.

Copy link

@Producdevity Producdevity left a comment

Choose a reason for hiding this comment

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

Just some suggestions, hope you don't mind

Comment on lines +88 to +90
except (UnicodeDecodeError, exceptions.ScraperError) as e:
logger.log_debug(f"Params: {params}")
raise exceptions.ScraperError(f"Error encoding URL: {e}. ROM params: {params}")

Choose a reason for hiding this comment

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

Just a suggestion: since ScraperError has a log level of error, I believe it gets included in the log file even when the log level is set to info. This could lead to users accidentally leaking their passwords when they upload their log.txt file to Discord or include it in an issue. To mitigate this, I suggest filtering out sensitive information before logging. Here’s an updated (untested) code snippet:

    except (UnicodeDecodeError, exceptions.ScraperError) as e:
        safe_params = {k: v for k, v in params.items() if k not in ['devpassword', 'sspassword', 'ssid']}
        logger.log_debug(f"Params: {safe_params}")
        raise exceptions.ScraperError(f"Error encoding URL: {e}. ROM params: {safe_params}")

"output": "json",
"ssid": username,
"sspassword": password,
}
return urlunparse(urlparse(USER_INFO_URL)._replace(query=urlencode(params)))
except UnicodeDecodeError as e:

Choose a reason for hiding this comment

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

Similar as comment above

synopsis = game["response"]["jeu"].get("synopsis")
if not synopsis:
return None

synopsis_lang = config["synopsis"]["lang"]
synopsis_text = next(
(item["text"] for item in synopsis if item["langue"] == synopsis_lang), None
(item["text"] for item in synopsis if item.get("langue") == synopsis_lang), None

Choose a reason for hiding this comment

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

Not sure if there is a good reason not to do this or if this will even end up throwing an error if "text" doesn't exist, but just something I noticed and felt like it might help if I pointed it out. (Disclaimer: I am not too familiar with python)

Suggested change
(item["text"] for item in synopsis if item.get("langue") == synopsis_lang), None
(item.get("text") for item in synopsis if item.get("langue") == synopsis_lang), None

Comment on lines 235 to 237
synopsis = game["response"]["jeu"].get("synopsis")
if not synopsis:
return None

Choose a reason for hiding this comment

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

Another suggestion that might not make sense, but just in case the api response is not what you expect it to be, could this be useful to do?

response = game.get("response", {})
jeu = response.get("jeu", {})
synopsis = jeu.get("synopsis")
if not synopsis:
    return None

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