Skip to content
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

Part of #3928 [Web Examples] Add First Class Python Support #4107

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

himanshumahajan138
Copy link
Contributor

@himanshumahajan138 himanshumahajan138 commented Dec 11, 2024

Pull Request

Adds First Class Python Support [Web Examples]
Part of #3928

Description

Web Examples for Python 1-hello-flask, 2-todo-flask, 3-hello-django, 4-todo-django Examples

Related Issues

Checklist

  • 1-hello-flask
  • 2-todo-flask
  • 3-hello-django
  • 4-todo-django
  • Updated Documentation

Status

Require Review!!!

@himanshumahajan138 himanshumahajan138 marked this pull request as ready for review December 12, 2024 19:54
@himanshumahajan138
Copy link
Contributor Author

@jodersky and @lihaoyi Please Review the code once...

Key Points:

  • Migrations automation(confused) Django Todo App
  • Structure of Django Code is maintained as per official structure
  • documentations will be updated after code acceptance
  • Code is Fully tested and require Reviews about structure and additional features

@himanshumahajan138 himanshumahajan138 changed the title [WIP] Part of #3928 [Web Examples] Add First Class Python Support Part of #3928 [Web Examples] Add First Class Python Support Dec 13, 2024
@himanshumahajan138
Copy link
Contributor Author

anyone up there please review...

@jodersky
Copy link
Member

I saw your request and will review it

@himanshumahajan138
Copy link
Contributor Author

himanshumahajan138 commented Dec 16, 2024

@jodersky sir i think you are busy so now i am working on Docs and let's review after that finally ✨

Copy link
Member

@jodersky jodersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At a first glance this looks great, @himanshumahajan138! Could you please use python's style guide in all examples however? In particular method names in python should not be camelCase, but snake_case

@himanshumahajan138
Copy link
Contributor Author

himanshumahajan138 commented Dec 16, 2024

@jodersky Sir, Let's Finalize this ✨

@himanshumahajan138
Copy link
Contributor Author

himanshumahajan138 commented Dec 19, 2024

@lihaoyi sir i think @jodersky Sir is Busy but no worry, code is already satisfied and checked by him in first review and now only docs review is pending so i think whenever you are less busy then after reviewing we can merge this PR
Hope you Understand!

@jodersky
Copy link
Member

jodersky commented Dec 19, 2024 via email

@himanshumahajan138
Copy link
Contributor Author

hey, I'll go over it again today

That's great to hear from your side ✨

Copy link
Member

@jodersky jodersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! I'm not knowledgeable with Django, but what I see makes sense.

Thanks for waiting so long for the review

Comment on lines +14 to +15
object unitTest extends PythonTests with TestModule.Unittest
object integrationTest extends PythonTests with TestModule.Unittest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is super nitpicky, but I personally always find it a bit weird to use mixed case names in folders. If you'd like to follow what's done in some other areas of mill, I would suggest to rename unitTest => test and integrationTest => itest

But I'm only expressing a very subjective point of view, so feel free to ignore this if you prefer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants