Fix misleading messages and ensure correct behaviour of get functionality #533
Conversation
…w misleading messages and does not update the current files. Added logic in query_data.py to check whether the query was skipped or failed
…w misleading messages and does not update the current files. Added logic in query_data.py to check whether the query was skipped or failed
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. Also consider joining our bi-weekly Saturday dev syncs. It'd be great to have you! Maintainer checklist |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
First PR Commit Check
- The commit messages for the remote branch of a new contributor should be checked to make sure their email is set up correctly so that they receive credit for their contribution
- The contributor's name and icon in remote commits should be the same as what appears in the PR
- If there's a mismatch, the contributor needs to make sure that the email they use for GitHub matches what they have for
git config user.emailin their local Scribe-Data repo (can be set withgit config --global user.email "GITHUB_EMAIL")
|
Thanks so much for the PR, @harikrishnatp! I'm away for a short vacation till the 26th, but can take more of a look then 😊 @mhmohona and @axif0, could you two take a look at the contribution here and give some feedback please? @harikrishnatp would also like some support writing tests, which maybe you all could discuss and I could help finalize? Specifically it would be great to add tests to |
| @@ -216,7 +216,7 @@ def query_data( | |||
|
|
|||
| else: | |||
| print(f"Skipping update for {lang.title()} {target_type}.") | |||
| break | |||
| return {"success": False, "skipped": True} | |||
There was a problem hiding this comment.
Amazing work @harikrishnatp !!! All functionality works as andrew told here.
So, When user press for skip, it doesn't print the Updated data was saved in.... Really great.
src/scribe_data/cli/get.py
Outdated
| file.unlink() | ||
| else: | ||
| print(f"Skipping update for {language.title()} {data_type}.") | ||
| return {"success": False, "skipped": True} | ||
| query_data( |
There was a problem hiding this comment.
I have a suggetion to optimize the issue. See query_data returns {'success': False, 'skipped': True} if the process skips. So can we store this with a variable and called it where Updated data was saved in... called? like in the line here as if not all and not variable["skipped"] ?
Let me know if it seems good approach to you.. @harikrishnatp
There was a problem hiding this comment.
Thank you for the suggestion! @axif0. Yes, I agree with your approach, I have incorporated the changes by storing the result in a variable and adding a condition to check query_result["skipped"] before printing the message about saved updates.
| @patch("scribe_data.cli.get.Path.glob") | ||
| @patch("builtins.input", return_value="s") | ||
| def test_user_skips_existing_file(self, mock_input, mock_glob, mock_query_data): | ||
| mock_glob.return_value = [Path("./test_output/English/nouns.json")] |
There was a problem hiding this comment.
Nice to see that you have added test cases. You can add DocString in the function, So that we can know what you have added. As it seems in the get.py all DocString's are missing. You are welcomed to explore and add those.
Also You can follow the numpydoc document also. I also do mistake writing DocString in many cases. We can help each other..
Looking forward to your thoughts.
There was a problem hiding this comment.
Thank you for the feedback! I appreciate the suggestion about adding docstrings. I’m still gaining experience in writing tests, and since the other tests in the file didn’t include docstrings, I assumed it wasn’t a priority. Now I understand how important they are for clarity and maintainability, and I’ll make sure to add them moving forward.
I'll work on improving it!
|
Looks like all checks are passing. I'll get to the final review in the coming days, @harikrishnatp! Do you want to take a look at #534 as a next issue in the meantime? :) |
|
Sounds good, I'll take a look at the issue :) |
| print(f"Skipping update for {language.title()} {data_type}.") | ||
| return {"success": False, "skipped": True} | ||
|
|
||
| query_result = query_data( |
There was a problem hiding this comment.
Here, as you can see existing files check already done in line of query_result function. Can we reconsider by ignoring the check before calling the query_result method?
Contributor checklist
pytestcommand as directed in the testing section of the contributing guideDescription
The problem this PR fixes is, when a user has fetched and got the file(s) by using the
getfunctionality, and when the user enters the same command again, the user will be prompted to either overwrite the file or skip the command. When the user skips, the program will say that it skipped the functionality but will still show that the file was updated. I worked around the skipping logic and got it fixed. Along with that I found out that query_data.py file which fetches the data had some flaws file fetching. I introducedsuccessandskippedreturn values for the relevant cases to ensure no errors occur while fetching the data.I added the testing conditions to the test_get.py file which and it was passing them. I tested them manually in my CLI as well.
Related issue