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

Django cookbook #129

Merged
merged 7 commits into from
Nov 20, 2022
Merged

Django cookbook #129

merged 7 commits into from
Nov 20, 2022

Conversation

djnnvx
Copy link
Contributor

@djnnvx djnnvx commented Oct 16, 2022

As promised in #125, here is the cookbook with my basic setup for Django.

It also contains a little remark regarding Django's async ORM, as I've had issues making it work at first.

Please let me know if anything is superfluous or missing, or if there is anything I can do to improve this PR.

thanks :)~

specifies how to setup rocketry in a django/drf app, per recommandations
seen in #76.

also adds a little note about django's async ORM, as I've personnally
had issues making it work at first, so it might be helpful to others as
well.
might make the docs a bit clearer
@Miksus
Copy link
Owner

Miksus commented Oct 16, 2022

Thanks a lot for the contribution! I think this will be very helpful for many. Getting late and cannot properly function this late but I'll check during the next week if that's fine. I'm also working on the event mechanism which is quite an interesting feature.

I think this needs to be added to the cookbook index.rst. Do you want to do it or do mind you if I add it?

@codecov-commenter
Copy link

codecov-commenter commented Oct 16, 2022

Codecov Report

Base: 94.90% // Head: 94.90% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (ec8865e) compared to base (9a53ad6).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #129   +/-   ##
=======================================
  Coverage   94.90%   94.90%           
=======================================
  Files          80       80           
  Lines        4453     4454    +1     
=======================================
+ Hits         4226     4227    +1     
  Misses        227      227           
Impacted Files Coverage Δ
rocketry/session.py 95.72% <0.00%> (+0.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Miksus
Copy link
Owner

Miksus commented Oct 16, 2022

Hmm, it seems the docs building is not in CI for some reason but I think the docs generation fails due to the page missing in index.rst, but I'll also check it in the upcoming days 👍

If you wish to try to generate the docs:

pip install tox
tox -e docs

Then those should be in the docs/_build/html/...

@djnnvx
Copy link
Contributor Author

djnnvx commented Oct 17, 2022

Thanks, i'll fix it in the afternoon.

- makes it look better overall, and easier to read (using subtitles and all
  that). also fixes ``code_examples``

- calls it from the cookbook index, so that it can actually be reached
  by a user

.. note ::
You will only need to use ``sync_to_async`` if you use the asynchronous ORM. The usage is well documented in
`Django's documentation <https://docs.djangoproject.com/en/4.1/topics/async/>`_.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://docs.djangoproject.com/en/latest/topics/async/ doesn't point to anything, sadly

@djnnvx
Copy link
Contributor Author

djnnvx commented Oct 26, 2022

Hi, did you have any chance to look at this ?

@Miksus
Copy link
Owner

Miksus commented Oct 26, 2022

Hi, sorry, been quite busy recently. My days are quite full this week (including a bit of work on Saturday). Perhaps on Sunday evening I could have time to check and merge, or on Monday evening latest.

Copy link
Owner

@Miksus Miksus left a comment

Choose a reason for hiding this comment

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

Some formatting suggestions.

Comment on lines 24 to 27
help = "Setup the periodic tasks runner"

def handle(self, *args, **options):
app.run()
Copy link
Owner

Choose a reason for hiding this comment

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

Perhaps these should be intended. Seems like these are an attribute and a method for the class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this has been fixed in the latest commit! thanks

async def do_things():
...

class Command(BaseCommand):
Copy link
Owner

Choose a reason for hiding this comment

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

Where is the BaseCommand from? I suppose it should be also imported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this has been resolved in the latest commit

Comment on lines 78 to 81
help = "Setup the periodic tasks runner"

def handle(self, *args, **options):
app.run()
Copy link
Owner

Choose a reason for hiding this comment

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

I think these should also be intended.

Comment on lines 93 to 125
class TaskView(APIView):
def get(self, request):

"""
This function is not ran by our scheduler, and runs in a synchronous context in our example
"""

name = request.GET.get('name')
if not name :
return Response({
'error': 'missing a parameter (expected something like '
'?name=job_name )'
}, status=HTTP_400_BAD_REQUEST)

try:

task = getattr(tasklist, name)

loop = asyncio.new_event_loop()
asyncio.set_event_loop(loop)

loop.run_until_complete(task())

except Exception as err:

return Response({
'error': 'task failure',
'logs': f'Failed with: {str(err)}',
}, status=HTTP_500_INTERNAL_SERVER_ERROR)

return Response({
'message': 'successfully ran the task',
}, status=HTTP_200_OK)
Copy link
Owner

Choose a reason for hiding this comment

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

If you don't mind I could reformat (whitespaces) this a bit.

@Miksus
Copy link
Owner

Miksus commented Oct 31, 2022

I'll also try to change the wording and correct some minor typos a bit in some places later today/tomorrow if you don't mind.

@djnnvx
Copy link
Contributor Author

djnnvx commented Oct 31, 2022

I'll get it done by the end of the week. I'm taking some time off right now, for personal reasons. But I will get it done, no problem

@Miksus
Copy link
Owner

Miksus commented Oct 31, 2022

Don't worrying about it 🙂 you are contributing to open-source so I expect you to do it when you are on the mood for it. I'm not in hurry and Rocketry is a marathon.

To be honest, I haven't been in good mood for the past two weeks but hopefully I also get back on track this week.

@djnnvx
Copy link
Contributor Author

djnnvx commented Nov 4, 2022

This should be OK now, feel free to add any changes to wording or coding style. Please let me know if there is anything else I can do to make it better.

Integrate Django (or Django Rest Framework)
===========================================

This cookbook will use DRF (Django REST Framework) as an example, but the syntax is exaclty

Choose a reason for hiding this comment

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

sorry for bothering, "exaclty" should be fixed. I take the opportunity to thank you for this docs. If it'd be possible to handle this case too it would be great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll do it tomorrow. Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've read the comments you have posted on the issue, and I don't think it really applies to a get-started cookbook. Couldn't an endpoint be used for such a use-case ? If you really need to send arguments to your job manually, you can use the APIView example I've added under to run what your task calls with the required arguments, and it will otherwise run by itself.

Maybe I did not understand what you're trying to do from the issue, I remain open to making this change but I might need an explanation as to what exactly is it you're trying to do through this example.

Choose a reason for hiding this comment

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

Glad to have your opinion. If you like, have a look at this short code explaining the use case (which, IMHO, can be pretty common even if not appropriate for the get-started cookbook) https://github.com/paolodina/rocketry-django-usecase

@Miksus
Copy link
Owner

Miksus commented Nov 7, 2022

Sorry for being lazy (to be honest, I had a self-inflicted headache yesterday). Unfortunately, it seems the next three days will go with hobbies (and today feeling quite tired) but hopefully on Friday I have time the latest, or in the weekend.

@paolodina
Copy link

No worries at all, take your rest and stay safe.

@djnnvx djnnvx requested a review from Miksus November 17, 2022 04:32
@Miksus
Copy link
Owner

Miksus commented Nov 17, 2022

Sorry, I broke my promise and just was too interested in side projects. I scheduled time on Sunday at 10:00 (EET) to check this finally out.

Perhaps I should have a recurrent timeslot in my calendar to check Rocketry's issues and PRs to have some structure on handling those and avoid forgetting.

@djnnvx
Copy link
Contributor Author

djnnvx commented Nov 18, 2022

No worries, this is not very urgent. Take your time and enjoy your passion projects!

@Miksus
Copy link
Owner

Miksus commented Nov 20, 2022

Yep, I think this is fine. I spotted an extra space but I don't think that's bad enough not to approve (I can fix those myself if you don't mind). 👍

As said, I'm not a Django expert so if there are things to fix, let's handle it as a community :)

@Miksus
Copy link
Owner

Miksus commented Nov 20, 2022

And by the way, I'll add your name to the new contributors.md if you don't mind.

@Miksus Miksus merged commit 2348c90 into Miksus:master Nov 20, 2022
@djnnvx
Copy link
Contributor Author

djnnvx commented Nov 20, 2022

Thanks, that's very cool. You said you were working on an event mechanism ? Maybe I could add some docs regarding that as well, if you'd like.

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.

4 participants