-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Make use of Pathlib's "/" #2312
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
Conversation
b754dbc to
e124084
Compare
e124084 to
4659721
Compare
docs/models.py
Outdated
| "djangoproject", "static", "robots.docs.txt" | ||
| ) | ||
| robots_path = settings.BASE_DIR / "djangoproject" / "static" / "robots.docs.txt" | ||
| with open(str(robots_path)) as fh: |
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.
Do you want to use robots_path.open here like you did in test_models.py?
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.
Yep. Resolved with the latest force push (diff)
776ee4b to
c163034
Compare
bmispelon
left a comment
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 looks good, nice work! 🎸
I haven't tested it locally, but I'm willing to trust the CI.
c163034 to
6ff4d4d
Compare
It provides a cleaner and easier to read experience, comparing to .joinpath()
73ba309 to
33ac63b
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.
👍 This style isn't my personal preference, but the code looks good!
I did a quick scan, and it doesn't seem like anything else changed in the force push.
Coming from:
LOCALE_PATHSsetting #2213 (comment)It provides a cleaner experience and it's easier to read, comparing to
.joinpath()