feat: ExtendedEmbed with some convenience methods and removed pytz
#72
Conversation
|
This PR was not deployed automatically as @Alc-Alc does not have access to the Railway project. In order to get automatic PR deploys, please add @Alc-Alc to the project inside the project settings page. |
Alc-Alc
changed the title
ExtendedEmbed with some convenience methods and removed putzExtendedEmbed with some convenience methods and removed pytz
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Type: Enhancement
PR Summary: This pull request introduces the ExtendedEmbed class along with some convenience methods for working with embed fields in a more structured way. It also includes changes to the datetime handling by removing the dependency on pytz and using UTC directly. Additionally, it refactors the way embeds are created and used within the python.py module, making the code cleaner and more maintainable.
Decision: Comment
📝 Type: 'Enhancement' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
📝 Complexity: the changes are too large or complex for Sourcery to approve.
- Unsupported files: the diff contains files that Sourcery does not currently support during reviews.
General suggestions:
- Consider adding more comprehensive testing around the new
ExtendedEmbedclass and its methods to ensure they work as expected under various conditions. This includes testing for edge cases and ensuring that thedeepcopymethod creates a true deep copy of the embed. - Ensure that the refactoring and changes made do not introduce regressions, especially in the
pepscommand functionality. Integration tests that verify the correct embed structure and rendering of fields would be beneficial. - Given the removal of
pytzand the switch to usingUTCdirectly, it's important to verify that datetime objects returned by functions such asquery_all_pepsare still correctly timezone-aware and behave as expected. Updating or adding unit tests to cover these scenarios would be prudent. - The removal of
types-pytzfrom the pre-commit configuration indicates a shift away from thepytzlibrary. Ensure that datetime-related functionality, particularly those involving timezones, continues to work as expected with the new setup through thorough testing.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
| @@ -0,0 +1,56 @@ | |||
| from copy import deepcopy | |||
There was a problem hiding this comment.
issue (llm): I noticed that the new ExtendedEmbed class and related functionality are introduced in this pull request. However, I couldn't find any unit tests covering these new additions. It's crucial to ensure that the new methods (add_field_dict, add_field_dicts, from_field_dicts, and the deepcopy method) work as expected under various conditions. Could you please add unit tests for these methods, including tests for edge cases such as empty fields list, invalid field parameters, and ensuring deepcopy indeed creates a deep copy?
| minified_embed.add_field(name="Created", value=pep_details["created"], inline=True) | ||
| minified_embed.add_field(name="Python Version", value=pep_details["python_version"], inline=True) | ||
| minified_embed.add_field(name="Documentation", value=docs_field, inline=False) | ||
| fields = [ |
There was a problem hiding this comment.
suggestion (llm): The refactoring to use ExtendedEmbed and Field in python.py is a significant change. While this makes the code cleaner, we need to ensure it doesn't introduce regressions. Please add integration tests that verify the peps command still produces the correct embed structure, especially focusing on the correct rendering of fields and the embed's thumbnail. It would also be beneficial to test how the command handles missing or incomplete data for a PEP.
| @@ -414,7 +413,7 @@ async def query_all_peps() -> list[PEP]: | |||
| "status": PEPStatus(pep_info["status"]), | |||
| "type": PEPType(pep_info["type"]), | |||
| "topic": pep_info.get("topic", ""), | |||
| "created": datetime.strptime(pep_info["created"], "%d-%b-%Y").replace(tzinfo=pytz.utc), | |||
| "created": datetime.strptime(pep_info["created"], "%d-%b-%Y").replace(tzinfo=UTC), | |||
There was a problem hiding this comment.
suggestion (llm): The change from pytz.utc to UTC in the query_all_peps function seems to be a part of a larger refactoring effort. However, it's essential to ensure that this change does not affect the functionality of any components relying on the query_all_peps function. Could you please add or update unit tests to verify that the datetime objects returned by query_all_peps are still correctly timezone-aware and behave as expected in time calculations and comparisons?
| @@ -142,7 +142,6 @@ repos: | |||
| asyncpg-stubs, | |||
| polyfactory, | |||
| discord-py, | |||
| types-pytz, | |||
There was a problem hiding this comment.
suggestion (llm): The removal of types-pytz from the pre-commit configuration suggests a move away from the pytz library. This change should ideally be accompanied by tests to ensure that any datetime-related functionality in the project, especially those involving timezones, continues to work as expected with the new setup. Please ensure that existing tests cover this aspect well, or consider adding new tests if necessary.
* feat: add python pep embeds * Update src/byte/lib/utils.py * chore: clean up commons imports * feat: ``ExtendedEmbed`` with convenience methods (#72) feat: `ExtendedEmbed` and some convenience methods Co-authored-by: Alc-Alc <alc@localhost> * chore: 404 no pep found made ephemeral * feat: finalize pep embed format --------- Co-authored-by: Alc-Alc <[email protected]> Co-authored-by: Alc-Alc <alc@localhost>
Pull Request Checklist
Description
Close Issue(s)