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

Update README.rst #275

Merged
merged 4 commits into from
May 22, 2024
Merged

Conversation

satcomjimmy
Copy link
Contributor

No description provided.

@mgrdcm
Copy link
Contributor

mgrdcm commented May 18, 2024

Good catch! Probably don't even need to keep around the old style anymore, since nothing older than Django 3 is supported.

Maybe ditch this part altogether: https://github.com/bennylope/django-organizations/pull/275/files#diff-7b3ed02bc73dc06b7db906cf97aa91dec2b2eb21f2d92bc5caa761df5bbc168fR169-R180

And just keep the updated syntax block: https://github.com/bennylope/django-organizations/pull/275/files#diff-7b3ed02bc73dc06b7db906cf97aa91dec2b2eb21f2d92bc5caa761df5bbc168fR184-R194

@satcomjimmy
Copy link
Contributor Author

Noob level: I had no idea my PR was going to go to the parent repo. I thought that was just to update my fork, however, I'm glad to drop the v1 syntax and update the request.

Good catch! Probably don't even need to keep around the old style anymore, since nothing older than Django 3 is supported.

Maybe ditch this part altogether: https://github.com/bennylope/django-organizations/pull/275/files#diff-7b3ed02bc73dc06b7db906cf97aa91dec2b2eb21f2d92bc5caa761df5bbc168fR169-R180

And just keep the updated syntax block: https://github.com/bennylope/django-organizations/pull/275/files#diff-7b3ed02bc73dc06b7db906cf97aa91dec2b2eb21f2d92bc5caa761df5bbc168fR184-R194

Removed v1 references and expected imports
@@ -172,8 +172,8 @@ main application URL conf as well as your chosen invitation backend URLs:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
from django.urls import path, include

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry @satcomjimmy I did a bad job describing what I was suggesting. I think it's a great idea to still include this line!

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think @bennylope?

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 likely have some more edits to suggest in the future, so I can add it back on a future PR

@bennylope bennylope merged commit 61373ae into bennylope:master May 22, 2024
5 checks passed
@satcomjimmy satcomjimmy deleted the satcomjimmy-readme_update branch May 22, 2024 00:26
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