Add tests for utility functions (resolves #50) #51
Conversation
f-strings can format numbers to use a comma as a thousands separator. The `add_num_commas` & `num_add_commas` functions are now redundant.
* Unit tests for `utils` module * Edit some error messages * Add type annotations for `mypy` checks
Thank you for the pull request!The Scribe team will do our best to address your contribution as soon as we can. The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :) If you're not already a member of our public Matrix community, please consider joining! We'd suggest using Element as your Matrix client, and definitely join the General and Data rooms once you're in. It'd be great to have you! Maintainer checklist
|
|
The comment on line 199 of |
|
Thanks for sending this along, @m-charlton! I’ll try to get to the review in the coming days :) :) |
|
Hey @m-charlton 👋 Just FYI I’m a bit under the weather, so the review will take a bit longer. Apologies! |
|
No worries. Get well soon. In the meantime I'll start to have look at #48 |
|
Thanks so much, @m-charlton! :) Happy to answer any Wikidata related questions if needed 😊 |
There was a problem hiding this comment.
Is there any interest in moving this data out to say a JSON file? This file would be loaded once on module import and then the utils functions could interrogate this loaded object.
You know what? I do like the idea actually 🤔 I'd be fine with having an issue for this (we can hash out the details there) and then an accompanying PR 🙌🚀
|
|
||
| def test_get_language_qid_negative(): | ||
| with pytest.raises(ValueError) as excp: | ||
| _ = utils.get_language_qid("Newspeak") |
There was a problem hiding this comment.
Double plus good!
Please find enclosed language_meta_data.json. The description & used_by fields are comments. The languages field is where the data are held proper. Not sure about the placement of the remove & ignore fields.
Writen a working implementation. Currently refactoring.
There was a problem hiding this comment.
Appreciate the easter egg as well! 🐰🥚😊 Till now we just have a random three emoji selection from "🥳🎉" as the gender annotation in the app when the user types "Scribe", and when you click the annotation it the selection is redone 🙃 It's fun 😇
| def test_get_scribe_languages(): | ||
| test_case = unittest.TestCase() | ||
|
|
||
| # test for content, not order |
There was a problem hiding this comment.
Minor nit here, @m-charlton: for comments we try to do capitalized and fully punctuated if it's its own line, and for inline comments they're not capitalized and have no period as it's here :) Happy to take care of this though!
| test_case = unittest.TestCase() | ||
|
|
||
| # test for content, not order | ||
| test_case.assertCountEqual( |
There was a problem hiding this comment.
You'd asked about this in the commit, and to me it's 100% fine! Thank you for giving this some though!
|
|
||
| def test_get_language_words_to_ignore_negative(): | ||
| with pytest.raises(ValueError) as excp: | ||
| _ = utils.get_language_words_to_ignore("JAVA") |
There was a problem hiding this comment.
Likely never will be in this here organization 😅
|
|
||
|
|
||
| def test_get_path_from_et_dir(): | ||
| # TODO: file path is same as above. Is this correct? |
There was a problem hiding this comment.
Yes it is. I think at one point they were at different levels, but they're in the same directory now. We could change this to get_path_from_scribe_data_child_dir and replace both?
|
This is all looking really great here, @m-charlton! 😊 Sounds like there's a bit more work to get the JSON up and running, but I'm happy to approve and merge once that's finalized 🚀 |
|
@andrewtavis could you open an issue for the loadable language data file idea? I've a couple of questions concerning data file format & directory/package layout. |
|
@m-charlton I created #52 for the data file idea 🚀 P.S. I'm okay with continuing the discussion there so this PR can get merged. Some refactoring will inevitably take place, as @m-charlton has started on already - so that new work can be tracked there 😄 |
Contributor checklist
Description
This PR addresses #50 by adding unit tests for the
utilsmodule as well as a few minor refactors:mypyget_language_words_to_ignore()&get_language_words_to_remove(). This puts them in line with other functions in the module.assertCountEqual(), where list comparisons are involved, as I'm assuming that list order is not important. Please correct me if I'm wrong.All these changes are in the second commit.
The first commit is the removal of the
add_num_commas()&num_add_commas()functionsAlthough code coverage is 100%. I'm looking for feedback on test coverage, especially for
check_command_line_args()&check_and_return_command_line_args()Related issue
Noticed that there is a lot duplication of the same language data distributed throughout utils.py. Is there any interest in moving this data out to say a JSON file? This file would be loaded once on module import and then the utils functions could interrogate this loaded object.
I'm willing to go into more detail and/or write a PR.