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 syntax error and type mismatch #43

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

Conversation

intls
Copy link
Contributor

@intls intls commented Dec 28, 2024

Description

  1. Syntax Error - A closing curly brace}is missing in the f-string on line 41. This results in a syntax error because Python expects the expression inside the curly braces to be completed and the quotation marks to be closed.

  2. Type Mismatch - The variables TEST, FETCH, and NO_CACHE are initialized using the env_vars function, which returns string values (e.g., "true" or "false"). However, in the ExecConf class, these variables are annotated as bool. Assigning string values to boolean attributes leads to a type mismatch.

If the string values are converted to booleans before assignment, this ensures that the test, fetch, and no_cache attributes will have the bool type, consistent with their annotations.

@sanchitram1
Copy link
Member

  1. I don't think a } is missing. Your PR adds a ), so it renders right
  2. The env_vars function returns boolean values. The purpose was to centrally handle all the different acceptance values (like 1, or true) over there. I think we can add the return type to the function.

core/config.py Outdated Show resolved Hide resolved
@intls intls requested a review from sanchitram1 January 3, 2025 03:19
Copy link
Member

@sanchitram1 sanchitram1 left a comment

Choose a reason for hiding this comment

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

@intls – could you also add the return type to the env_vars function in core.utils? I think that might prevent this confusion about the return types for future contributors.

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