-
Notifications
You must be signed in to change notification settings - Fork 1
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
Create form for create and update post #32
Conversation
@webknjaz we can merge this Django project in master? |
No |
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.
Let's do rebase and commits cleanup in each PR before merging from now on.
blog/forms.py
Outdated
"""Create and edit form.""" | ||
|
||
class Meta: | ||
"""Build form on the model and with the fields.""" |
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.
It's not an action.
blog/forms.py
Outdated
|
||
|
||
class PostForm(forms.ModelForm): | ||
"""Create and edit form.""" |
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.
It's not an action, but entity.
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.
@webknjaz Creating and editing forms.
? Is good doc for this class
?
blog/templates/blog/base.html
Outdated
href="//maxcdn.bootstrapcdn.com/bootstrap/3.2.0/css/bootstrap.min.css"> | ||
<link rel="stylesheet" | ||
href="//maxcdn.bootstrapcdn.com/bootstrap/3.2.0/css/bootstrap-theme.min.css"> | ||
<link href='//fonts.googleapis.com/css?family=Lobster&subset=latin,latin-ext' |
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.
Don't mix different quotation styles.
blog/templates/blog/post_detail.html
Outdated
{% endif %} | ||
{% if user.is_authenticated %} | ||
<a class="btn btn-default" | ||
href="{% url 'post_edit' pk=post.pk %}"><span |
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.
Reformat this with the hierarchy in mind, not just do a new line on random space.
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.
@webknjaz Sorry?
@webknjaz I fixed all problems. |
f05021a
to
ddb151f
Compare
blog/views.py
Outdated
@@ -1,3 +1,50 @@ | |||
"""Views for the blog app.""" | |||
from django.shortcuts import render, get_object_or_404 | |||
from django.utils import timezone | |||
from .models import Post |
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.
Don't mix app imports with framework imports.
blog/views.py
Outdated
|
||
def post_list(request): | ||
"""Show list posts on the page.""" | ||
posts = Post.objects.filter(published_date__lte=timezone.now()).order_by( |
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.
Better readable:
posts = (
Post.objects.
filter(published_date__lte=timezone.now()).
order_by('-published_date')
)
blog/templates/blog/post_list.html
Outdated
{% for post in posts %} | ||
<div class="post"> | ||
<div class="date"> | ||
<p>Опубликовано: {{ post.published_date }}</p> |
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.
Please replace all translatable strings with i18n ({% trans "Posted on:" %}
).
This requires {% load i18n %}
.
blog/templates/blog/post_detail.html
Outdated
{% if user.is_authenticated %} | ||
<a class="btn btn-default" | ||
href="{% url 'post_edit' pk=post.pk %}"><span | ||
class="glyphicon glyphicon-pencil"></span></a> |
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.
make this nicer
blog/templates/blog/base.html
Outdated
<div class="page-header"> | ||
{% if user.is_authenticated %} | ||
<a href="{% url 'post_new' %}" class="top-menu"><span | ||
class="glyphicon glyphicon-plus"></span></a> |
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 kind of nesting is unreadable
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.
And add pyflakes to pre-commit.
@webknjaz Sorry,
from pre-commit.com/hooks.html |
It uses only a limited subset of pyflakes. try pylint then |
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.
use i18n in all other places.
I need to create a new branch or add |
Let's do it here |
blog/templates/blog/base.html
Outdated
@@ -1,4 +1,7 @@ | |||
{% load staticfiles %} | |||
|
|||
{% load i18n %} |
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.
Wrap all strings.
pre-commit/pre-commit-hooks#157 - Do I need to use a local hook? - https://github.com/Yelp/venv-update/blob/de9e1befdecf448cf02ceb5c57a30bcc88a58c06/.pre-commit-config.yaml#L30-L36 |
7aab1c1
to
564a3b3
Compare
@webknjaz Hi. What do you say about the code? Good now or still bad? |
.travis.yml
Outdated
before_install: | ||
- pip install -U pre-commit | ||
- pip install pylint-django==0.11.1 | ||
- pip install -r requirements.txt |
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.
Why did you split just one requirement into a file, while listing others outside of it? Be consistent.
clean-up your commits |
c5bec2c
to
cd22a76
Compare
@webknjaz Now is good? |
.pre-commit-config.yaml
Outdated
"".join([os.getenv("HOME"), "/virtualenv/python", platform.python_version(), | ||
"/lib/python", ".".join(platform.python_version_tuple()[:2]), "/site-packages"]) | ||
if os.getenv("TRAVIS") else | ||
"/home/sergey/.virtualenv/django_girls/lib/python3.6/site-packages".format(os.getcwd()) |
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.
NEVER hardcode paths. Also, you inject current working directory from os.getcwd()
to nowhere.
blog/templates/blog/post_edit.html
Outdated
{% load i18n %} | ||
|
||
{% block content %} | ||
<h1>New post</h1> |
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.
i18n.
blog/views.py
Outdated
from .models import Post | ||
|
||
|
||
def post_list(request): |
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.
cd22a76
to
3dfa0a5
Compare
@webknjaz done. |
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.
not done.
b7a9472
to
85f8f16
Compare
4b1a716
to
2eeb8fb
Compare
@webknjaz Done. |
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.
fix docstrings
blog/forms.py
Outdated
"""Class form based on the model.""" | ||
|
||
class Meta: | ||
"""Description metadata.""" |
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.
which description?
blog/forms.py
Outdated
|
||
|
||
class PostForm(forms.ModelForm): | ||
"""Class form based on the model.""" |
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.
i see that it's a class, it's stated one line above. anything useful you want to tell me with this?
blog/forms.py
Outdated
@@ -0,0 +1,14 @@ | |||
"""Creating and editing forms.""" |
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.
Is it a prose or a module description? It's not an action, it's a module.
mysite/urls.py
Outdated
@@ -4,5 +4,5 @@ | |||
|
|||
urlpatterns = [ | |||
path('admin/', admin.site.urls), | |||
path(r'', include('blog.urls')), | |||
path('', include('blog.urls')), |
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.
How is this change related to this PR?
2eeb8fb
to
32494dd
Compare
@webknjaz what do you think now about the code? |
I think that you need to read all comments, not just ones you like. |
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.
Fix comments
blog/forms.py
Outdated
@@ -0,0 +1,14 @@ | |||
"""The module has forms who saved in DB and will be shown in HTML file.""" |
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.
Mind your grammar
blog/forms.py
Outdated
|
||
|
||
class PostForm(forms.ModelForm): | ||
"""Form for our Post model.""" |
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.
"our"? whose?
blog/forms.py
Outdated
"""Form for our Post model.""" | ||
|
||
class Meta: | ||
"""Description Post's metadata.""" |
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.
Docstring is a description, it's obvious.
32494dd
to
acdd1fb
Compare
blog/forms.py
Outdated
"""Form for Post model.""" | ||
|
||
class Meta: | ||
"""Configuration for form.""" |
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.
of
blog/forms.py
Outdated
|
||
|
||
class PostForm(forms.ModelForm): | ||
"""Form for Post model.""" |
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.
Forget about model, it's about visual effect.
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.
same
b4492f1
to
1c7207c
Compare
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.
Now, fix the last comment left.
7c48045
to
74ea5eb
Compare
No description provided.