Conversation
…nts to organise from the end user
|
@paurushofficial please list the changes here, also make the title as concise as you can. |
sumit4613
left a comment
There was a problem hiding this comment.
Thanks for the PR, but you should fix the suggested changes.
events/forms.py
Outdated
| fields = ['title', 'student_name', | ||
| 'email_id', 'mobile_number', 'roll_no', 'branch'] | ||
|
|
||
| class requestEventForm(ModelForm): |
There was a problem hiding this comment.
Issues here :
- You're not following PEP8 conventions here.
- According to PEP8 there should be 2 blank lines after a class.
- Whenever you create class, it should be in caps. eg.
class RequestEventForm(ModelForm) - Also try to include docstrings in your class/methods.
events/models.py
Outdated
| def __str__(self): | ||
| return self.student_name | ||
|
|
||
| class requestEvent(models.Model): |
There was a problem hiding this comment.
- Here also, In classes, you should follow
CamelCasenotation. - Try to add docstrings to explain what the class/method is doing.
- You can also add docstrings for existing classes/methods.
events/views.py
Outdated
| #numberOfEvents = len(events) | ||
| #if (numberOfEvents) == 0: | ||
| # events = 0 |
There was a problem hiding this comment.
Why did you left commented code here. You should remove the unused code.
events/views.py
Outdated
| def requestEvent(request): | ||
| if request.method == 'POST': | ||
| form = requestEventForm(request.POST) | ||
| if form.is_valid(): | ||
| form.save() | ||
| your_name = form.cleaned_data.get('your_name') | ||
| title = form.cleaned_data.get('title') | ||
| messages.success( | ||
| request, f'Thank you { your_name } for requesting to organise { title }, you will be contacted shortly!') | ||
| return redirect('homepage') | ||
| else: | ||
| form = requestEventForm() | ||
| return render(request, 'event_register.html', {'form': form}) |
There was a problem hiding this comment.
While creating methods, you should not write like this.
Use this notation, def request_event(request):
|
This pull request introduces 1 alert when merging d0fe5cf into a8904e0 - view on LGTM.com new alerts:
|
|
@paurushofficial Sorry for being this late on replying to your PR. Could you please share some screenshots so that I can see it's working as expected. |
…nts to organise from the end user