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 #565 by raising an Exception for an empty String #566

Merged
merged 4 commits into from
Mar 4, 2019

Conversation

bharatr21
Copy link
Contributor

Fix #565 by raising a ValueError for an empty string.

@alanakbik
Copy link
Collaborator

Great, thanks for adding this!

@alanakbik
Copy link
Collaborator

👍

@stefan-it
Copy link
Member

@Bharat123rox Thanks! Could you also add a small unit test for that case (=empty sentence)? That would be great :)

@alanakbik
Copy link
Collaborator

@stefan-it good point! Once the unit test is there, could you +1 and merge?

@stefan-it
Copy link
Member

Sure, no problem :)

@bharatr21
Copy link
Contributor Author

bharatr21 commented Mar 2, 2019

@stefan-it Should I add a separate function in test_data.py or under one of the test_create_sentence_without_tokenizer() or test_create_sentence_with_tokenizer() functions? Also, since I have to check for an exception, do I use

with pytest.raises(ValueError) as e:
     assert(e.info) == "My error message"

if not, how do I go about ensuring that it raises the required exception?
(Thanks for your help, I'm new to pytest)

@bharatr21
Copy link
Contributor Author

@stefan-it @alanakbik Will this be enough? How else should I test it?

@alanakbik
Copy link
Collaborator

Looks good to me! @kashif @MichaelHintz @stefan-it can one of you check, +1 and merge?

@bharatr21
Copy link
Contributor Author

@alanakbik In case it looks good to you, please approve this PR, the approval seems to get reset on each new commit made.

@stefan-it
Copy link
Member

👍

1 similar comment
@kashif
Copy link
Contributor

kashif commented Mar 4, 2019

👍

@kashif kashif merged commit c2c2b3f into flairNLP:master Mar 4, 2019
@kashif
Copy link
Contributor

kashif commented Mar 4, 2019

thanks @Bharat123rox

@bharatr21
Copy link
Contributor Author

thanks @kashif and @stefan-it for your patience

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.

4 participants