-
Notifications
You must be signed in to change notification settings - Fork 75
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
feat interactive v2.0 #500
Conversation
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 |
No need :) Is a great idea 😊 Will review over the weekend! |
Functionality wise this is looking really great, @axif0, but can we have interactive mode be per command, rather than adding it all onto |
…auto execute after PyICU installation
Modified |
src/scribe_data/cli/get.py
Outdated
@@ -136,7 +142,14 @@ def get_data( | |||
# MARK: Emojis | |||
|
|||
elif data_type in {"emoji-keywords", "emoji_keywords"}: | |||
generate_emoji(language=language, output_dir=output_dir) | |||
if not check_if_pyicu_installed(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By adding this user don't have to execute the command again. By adding os.execv
I learned it would replace the current Python process with the new script, passing the arguments.
PyTest is good in locally. But I don't know why I got 403 error
and EOFError
in github. so I reverted the code as previous.
Generally things are looking good, @axif0, but I'm getting a 403 forbidden for the query from total interactive mode. Might just be the server. Could I also ask you to take a look at the changes I made in my commit and familiarize yourself with the comment formatting - i.e. own line comments are capitalized and punctuated, and inline comments are lower case and don't have a period :) |
Awesome, I've reviewed the changes you mentioned. You're right, following the standard comment formatting is important. Thanks for the guidance - I appreciate you taking the time to ensure I'm aware of these best practices. I'll apply the capitalization, punctuation, and formatting standards you described in future. Also, Is 403 error fixed now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All's working now, @axif0 :) Likely just the Wikidata Query Service needing us to back off for a moment. Thanks so much for these improvements! This really has come along so well 😊
Contributor checklist
pytest
command as directed in the testing section of the contributing guideDescription
Added test for interactive, get
Added total and list functionalities
Updated command messages
Added prefilled language, data-types functionalities.
Related PR