Skip to content
/ django Public

Comments

Fixed #29983 -- Replaced os.path() with pathlib.Path in project template and docs.#12005

Merged
carltongibson merged 1 commit intodjango:masterfrom
jdufresne:pathlib
Nov 7, 2019
Merged

Fixed #29983 -- Replaced os.path() with pathlib.Path in project template and docs.#12005
carltongibson merged 1 commit intodjango:masterfrom
jdufresne:pathlib

Conversation

@jdufresne
Copy link
Member

@jdufresne jdufresne commented Nov 2, 2019

ngnpope
ngnpope suggested changes Nov 2, 2019
View reviewed changes
@jdufresne
Copy link
Member Author

jdufresne commented Nov 2, 2019

Thanks for the review! I've made the suggested simplifications and included a note in the release notes. If you have suggestions on the wording, let me know.

ngnpope
ngnpope approved these changes Nov 4, 2019
View reviewed changes
carltongibson
carltongibson reviewed Nov 5, 2019
View reviewed changes
Copy link
Member

@carltongibson carltongibson left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @jdufresne. Thanks again for this. In theory, yes great.

Can you see @claudep's comment on the ticket:

I'm not sure if various Django settings requiring a path (template DIRS, STATIC/MEDIA_ROOT, etc.) all support path inputs. Shouldn't we check and add tests for those before changing the project settings template?

We have had regressions pop-up from moving the Path. This is probably sensible to audit and add coverage. What do you think?

@jdufresne
Copy link
Member Author

jdufresne commented Nov 5, 2019

Makes sense. I'll try to identify all settings that could use a Path and either check if a test exists or add one. I'll report back once complete.

@carltongibson
Copy link
Member

carltongibson commented Nov 6, 2019

I'll report back once complete.

Hey @jdufresne — Thanks for all the related PRs. Great stuff. More incoming?

@jdufresne
Copy link
Member Author

jdufresne commented Nov 6, 2019

Hi Carlton, I have a few more settings to verify before rebasing this PR. Just one example is LOCALE_PATHS, but there may be others. I'll comment here when I believe all are covered.

@jdufresne
Copy link
Member Author

jdufresne commented Nov 7, 2019

All settings now have a test in PR #12042.

@claudep
Copy link
Member

claudep commented Nov 7, 2019

Awesome work!

@carltongibson carltongibson self-assigned this Nov 7, 2019
@jdufresne @carltongibson
Fixed #29983 -- Replaced os.path() with pathlib.Path in project templ…
d0eb43d 
…ate and docs.

Thanks Curtis Maloney for the original patch.
@carltongibson carltongibson changed the title Fixed #29983 -- Replaced os.path with pathlib.Path. Fixed #29983 -- Replaced os.path() with pathlib.Path in project template and docs. Nov 7, 2019
carltongibson
carltongibson approved these changes Nov 7, 2019
View reviewed changes
Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Great stuff. Thank you for your sterling work on this @jdufresne! 🥇

@carltongibson carltongibson merged commit 26554cf into django:master Nov 7, 2019
@jdufresne jdufresne deleted the pathlib branch November 8, 2019 10:13
@luzfcb luzfcb mentioned this pull request Jul 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@ngnpope ngnpope ngnpope approved these changes

@carltongibson carltongibson carltongibson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@carltongibson carltongibson

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jdufresne @carltongibson @claudep @ngnpope