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

Improve project composition #2

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Improve project composition #2

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 25, 2019

Hello again
I just modified the project's external structure and did not delete or modify any major components in the project

@piotr-kopacki
Copy link
Owner

Hello again!

I'm actually very impressed with that layout, I've never seen a project structure like that.
Were you inspired by any existing projects or books, or is it actually your idea?

Don't have time for a proper review, but at a first glance I noticed you haven't run pre-commit run on those changes (I've added it to Contribution section in README). Can you fix it please?

Thank you!

@ghost
Copy link
Author

ghost commented Dec 25, 2019

This project using this structure https://github.com/Hopetree/izone
I’m using pre commit but return nothing
I don’t like to use poetry, I think no one use it

@ghost
Copy link
Author

ghost commented Dec 25, 2019

and I’m build this website using same structure
http://code4ever.herokuapp.com
it amazing and let any one to understand your project and support you if you need help

@piotr-kopacki
Copy link
Owner

pre-commit works only when files are staged. After you've commited you have to run pre-commit run --all-files

Also, I added Poetry to make package management easier. It is optional for users as we're also sharing requirements.txt.

from notifications.models import Notification


User = get_user_model()
Copy link
Owner

Choose a reason for hiding this comment

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

User is not used in this file, please remove it.

Copy link
Author

Choose a reason for hiding this comment

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

ok i'll do that

@@ -1,6 +1,12 @@
from django.contrib.auth import get_user_model
Copy link
Owner

Choose a reason for hiding this comment

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

User model is not used in this file, please remove it.

Copy link
Author

Choose a reason for hiding this comment

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

yes you're right

"allauth",
"allauth.account",
"allauth.socialaccount",
"rest_auth.registration",

"entries.apps.EntriesConfig",
'messeges.apps.MessegesConfig',
Copy link
Owner

Choose a reason for hiding this comment

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

Typo messeges -> messages

Copy link
Author

Choose a reason for hiding this comment

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

you cant use this because django already use it Messages App

Copy link
Owner

Choose a reason for hiding this comment

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

Well then you have to come up with a new name :D
Maybe private_messages?

Copy link
Author

Choose a reason for hiding this comment

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

I prefer not to use the _ to create Django applications. We can change the model name to Message

Copy link
Owner

Choose a reason for hiding this comment

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

Model name is good, I'm not fine with a typo in apps name. If not private_messages then maybe inbox, private, conversations, etc.

Copy link
Owner

@piotr-kopacki piotr-kopacki left a comment

Choose a reason for hiding this comment

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

Looks good overall but still not sure about app names being plural i.e. users not user but I guess I could live with that.

I left some minor comments that should be addressed so when these are resolved and pre-commit is applied I will accept this PR.

Thank you!

@ghost
Copy link
Author

ghost commented Dec 26, 2019

Looks good overall but still not sure about app names being plural i.e. users not user but I guess I could live with that.

I left some minor comments that should be addressed so when these are resolved and pre-commit is applied I will accept this PR.

Thank you!

I did it depending on the structure of the Django applications, for example
django sites app, django staticfiles app, django flatpages app, django sitemaps app ext....

@piotr-kopacki
Copy link
Owner

Looks good overall but still not sure about app names being plural i.e. users not user but I guess I could live with that.
I left some minor comments that should be addressed so when these are resolved and pre-commit is applied I will accept this PR.
Thank you!

I did it depending on the structure of the Django applications, for example
django sites app, django staticfiles app, django flatpages app, django sitemaps app ext....

Understood, as I said I can live with that :)

@piotr-kopacki
Copy link
Owner

Hey, I see you've hidden/removed your fork repository. Should I remove the PR?
That would be a bummer since I was excited about the changes.

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.

1 participant