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

Create a blog Django app #20

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

Conversation

Martolivna
Copy link
Collaborator

@Martolivna Martolivna commented Sep 8, 2023

This patch adds a blog app. The blog app contains a blog post model and related data base migrations, tests.

Ref #16

@OlenaYefymenko
Copy link
Member

@Martolivna, PR looks correct, taking into account this step https://tutorial.djangogirls.org/en/django_models/.
I've added some minor comments. However, it would be better to provide a more detailed description of the PR, 1–2 sentences about models and migration.
As far as I know, the next step will be adding test, so it's would be better to clarify each part of PR.

@webknjaz
Copy link
Member

webknjaz commented Sep 9, 2023

@OlenaYefymenko publish your review

Copy link
Member

@OlenaYefymenko OlenaYefymenko left a comment

Choose a reason for hiding this comment

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

@Martolivna, PR looks correct, taking into account this step https://tutorial.djangogirls.org/en/django_models/.
I've added some minor comments. However, it would be better to provide a more detailed description of the PR, 1–2 sentences about models and migration.
As far as I know, the next step will be adding test, so it's would be better to clarify each part of PR.

blog/models.py Show resolved Hide resolved
blog/models.py Outdated Show resolved Hide resolved
Comment on lines +1 to +3
# VS Code
.vscode

Copy link
Member

Choose a reason for hiding this comment

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

There are a few problems here:

  1. Ignoring VS Code is usually more involved and includes allow-listing certain config files that are supposed to be checked into Git: https://www.toptal.com/developers/gitignore/api/visualstudiocode
  2. Editor-specific ignores are considered out of the scope in individual repos and are typically recommended to be managed through user-global exclusions: https://sebastiandedeyne.com/setting-up-a-global-gitignore-file
  3. Finally, changing Git configurations that are not directly related to the topic of the PR, make it non-atomic. So they must be excluded from the patch and submitted as separate PRs should the need be.

from django.utils import timezone


class Post(models.Model):
Copy link
Member

Choose a reason for hiding this comment

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

@Martolivna I want you to write tests for this model.

Create a tests/test_models.py file, import this model there, follow https://docs.djangoproject.com/en/4.2/topics/testing/overview/ to declare a test case class and make dedicated methods for every separate thing/aspect you're testing.
Generally, those tests would make an instance of this class and then, call its methods + check their return values or side effects.

Make sure to call every method you declared in this class.

Bonus: you can also integrate coverage reporting like so https://adamj.eu/tech/2019/04/30/getting-a-django-application-to-100-percent-coverage/.

blog/models.py Outdated Show resolved Hide resolved
blog/models.py Outdated Show resolved Hide resolved
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.

3 participants