-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Support for Mongoengine in V2.0 #2611
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
Conversation
Only known Problem yet is with the Widgets, but that could be related to an outdated wtforms version. Also waiting for the tests. |
Note: you may want to run the style tests before making the commit (they auto-fix the code style). I believe it can be done via |
|
Oh thanks, this command changed a lot :) |
About the last Tests, Typing, my guess would be to change the requirements/typing.in to add mongoengine? |
I imagine you need to add (Or possibly |
Yep @LeXofLeviafan is right - if we're adding mongoengine back, we need to add it to our dependency files. This can be a bit of a pain because of how we/pip manages them across multiple files (and probably a good incentive to switch to something more like Let me know if you need a hand and I can try to help (/work out exactly what to do). |
So… any news on this yet? |
If someone could help me with the final check would be nice. Don't want to break something. Then everything would be done :) |
Samuel, can you update or tell us how to update the dependencies for type checking? or can I just try to add mongoengine to |
@samuelhwilliams I believe this was a question for you 😅 @ElLorans based on the comment within that file, the correct answer appears to be "place |
Argh very sorry - thank you for the nudge. Let me give it a go now - it's quite awkward at the moment to get all of the versions of things in different files compatible. More justification for us/me to switch us over to uv ASAP, probably. |
re: type hints, this issue on mongoengine suggests that the package doesn't really have type hinting support. It mentions an alternative, We might want to just disable type hinting on the mongoengine (for now / forever) package by adding it to the In this case no updates will be needed to the requirements files so avoids that sometimes-painful step. But for future reference the principle is vaguely:
|
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.
This will also need some amount of test coverage when you get happy with the functionality :)
@Bastian-Kuhn …Any news on this? 😅 |
Hello @LeXofLeviafan Yes, currently just the Exception from here left: #2541 which I was hoping for help but of course will try to debug it again. And then about testing where someone needs to decide how to go on, to either ignore the typing hints or that I should update the requirements file like described. |
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.
Regarding the testing typehints: if I got what @samuelhwilliams said right, you could try mongo-types
but if that doesn't work out the only other option would be to turn them off for mongo.
I resolved all open conversations, so if the other contributors approve the new edits and have nothing else to add, no. |
Approving would be tricky for me… since I don't have access to do so 🙃 That being said, I don't have any new comments regarding the code; so I guess what's left is to get approval from @samuelhwilliams …Is this the last item to tackle before (finally!) completing the 2.0 release? Or will someone pick up the |
Thanks for the nudge. I've been following this loosely but if we think it's ready to go now, I'll have a last look and play and see if I can find any outstanding blockers. If not then let's get it merged and hopefully get 2.0 over the line 🤞 |
@samuelhwilliams I believe including a static screenshot would be a big help when trying to investigate 😅 |
Fair points, sorry - but hopefully the instructions on how to reproduce are quite straight-forward :) |
TBH, I liked the GIF. I implemented a bugfix, but I think we need a unit test for this. @Bastian-Kuhn, I removed |
Nice, have checked out and agree that seems to have fixed it @ElLorans 👍 A test would be nice, agreed. |
I added a simple one:
|
I think I'm happy enough at this point. Can I get a few other reviews as well please? :) Let's plan to merge this by the end of the week and then we should be in a place to do a v2.0.0 rc release - if not the full thing. 👍 🙇 Note that, for better or worse, I have mainly focused on a usability review rather than a line-by-line code review. |
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.
Pull Request Overview
This PR adds support for MongoEngine in Flask-Admin v2.0 by removing the dependency on flask-mongoengine while maintaining full functionality for MongoDB document management.
- Adds mongoengine optional dependency configuration
- Updates translation files with new MongoEngine-specific strings
- Restores MongoEngine support without flask-mongoengine dependency
Reviewed Changes
Copilot reviewed 58 out of 96 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
pyproject.toml | Adds mongoengine optional dependency and build configuration |
flask_admin/translations/**/admin.po | Updates translation files with MongoEngine filter and view messages |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
Great job @Bastian-Kuhn, thank you for contributing 🙏.
Note that, for better or worse, I have mainly focused on a usability review rather than a line-by-line code review.
I did the same, the example looks great 🤓.
Thanks everyone (@Bastian-Kuhn @LeXofLeviafan @ElLorans @hasansezertasan) for the huge effort involved in getting this support back in 🙇 Feels really good to be able to keep this. |
Thanks to @karpitsky this brings back the support for Mongoengine, but without the dependency of flask-mongoengine.
Reference: #2541