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

asserts replaced by exception for text classification task with test. #3256

Merged

Conversation

manisnesan
Copy link
Contributor

@manisnesan manisnesan commented Nov 12, 2021

I have replaced only a single assert in text_classification.py along with a unit test to verify an exception is raised based on #3171 .

I would like to first understand the code contribution workflow. So keeping the change to a single file rather than making too many changes. Once this gets approved, I will look into the rest.

Thanks.

Copy link
Collaborator

@mariosasko mariosasko 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 working on this! Just one suggestion:

Copy link
Member

@lhoestq lhoestq 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 contribution and for taking care of writing a test :)

I just did a minor modification in the test:

tests/test_tasks.py Outdated Show resolved Hide resolved
tests/test_tasks.py Outdated Show resolved Hide resolved
@lhoestq
Copy link
Member

lhoestq commented Nov 12, 2021

Haha it looks like you got the chance of being reviewed twice at the same time and got the same suggestion twice x)
Anyway it's all good now so we can merge !

@lhoestq lhoestq merged commit bf2d230 into huggingface:master Nov 12, 2021
@manisnesan
Copy link
Contributor Author

Thanks for the feedback.

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.

3 participants