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

Check answers for integer and float #173

Merged

Conversation

g-normand
Copy link

No description provided.

@Pierre-Sassoulas Pierre-Sassoulas added the enhancement This is a feature not a bug label Aug 31, 2022
Copy link
Owner

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Looks reasonable thank you ! Could you add covering tests for it, please ?

@Pierre-Sassoulas Pierre-Sassoulas added this to the 1.4.4 milestone Aug 31, 2022
@g-normand
Copy link
Author

@Pierre-Sassoulas I added covering tests. Thanks for the quick reply

Copy link
Owner

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Thank you for adding tests ! The current issue with the pipeline is due to a function that became too complex survey/models/answer.py:57:5: C901 'Answer.check_answer_body' is too complex (14). Would you mind doing a refactor first ?

Maybe a visitor pattern like:

def check_answer_body(self, question, body):
    func = getattr("check_answer_body_for_{question.type.lower()}")
    return func(question, body)


def check_answer_body_for_integer(self, question, body):    
    if body and body != "":
        try:
            body = int(body)
        except ValueError:
            msg = "Answer is not an integer"
            raise ValidationError(msg)

@g-normand
Copy link
Author

@Pierre-Sassoulas I changed a little bit the function :)
Let me know if it's ok or not

Copy link
Owner

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Thank you for refactoring 👍

@Pierre-Sassoulas Pierre-Sassoulas merged commit 5e0a924 into Pierre-Sassoulas:main Sep 5, 2022
@Pierre-Sassoulas
Copy link
Owner

I released this in 1.4.4 it should be available shortly.

@g-normand
Copy link
Author

Perfect, thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is a feature not a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants