-
Notifications
You must be signed in to change notification settings - Fork 1
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
Revert PRs #21, #22 #23
Conversation
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.
Hey @MusicalNinjaDad - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
## Python 2.0.0 | ||
|
||
- **Breaking:** Changed top level module name to `fizzbuzz`. Use `from fizzbuzz.fizzbuzzo3 import fizzbuzz` | ||
for rust implementation or `from fizzbuzz.fizzbuzzpy import fizzbuzz` for python implementation (slower). |
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.
question: Clarification needed on removal of Python 2.0.0 changelog entry.
The removal of the entire section for Python 2.0.0 might confuse users looking for migration details. Consider keeping a summarized version or a link to migration documentation.
@@ -1,5 +1,4 @@ | |||
{ | |||
// "image": "ghcr.io/musicalninjas/pyo3-devcontainer:latest", // untested with mac, expect issues due to x_64 rust installation |
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.
question: Explanation needed for removal of devcontainer image comment.
If the image is now tested and compatible with Mac, documenting this change could be beneficial for developers using different environments.
@@ -1,4 +1,3 @@ | |||
name: Deploy Rust |
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.
question: Check if the removal of the workflow name was intentional.
Removing the name of a GitHub Actions workflow can affect readability and logging. If this was unintentional, it might be worth adding it back.
This reverts commits
c132883
869663a