-
Notifications
You must be signed in to change notification settings - Fork 1.1k
remove requirement_dev.txt from project #2277
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
remove requirement_dev.txt from project #2277
Conversation
f5cec10 to
b3bab6b
Compare
.readthedocs.yaml
Outdated
| commands: | ||
| - pip install --group docs |
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.
i think now this should work
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.
Thanks for adjusting things. Did you happen to test this?
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.
No, I haven't test this. bcs i have no idea to test this locally.
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.
I'm going to change this based on this part of the docs: https://docs.readthedocs.com/platform/stable/config-file/v2.html#build-commands
tim-schilling
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.
Thank you for iterating on this. I still have some open questions unfortunately. I'm sorry for the slow responses.
|
Hey @tim-schilling , Currently I have some thoughts in my mind.
I'm happy to iterate as per the feedback. and I'm happy answer/discuss questions. Thank you. |
b3bab6b to
9191aea
Compare
tim-schilling
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 is a different workflow than I'm used to, though I think it's close to a mergeable state. @matthiask or I will need to check on the readthedocs integration after this gets merged. It's probably a good idea regardless since it's been a while since I've looked at that.
I'd like to get opinions from @django-commons/django-debug-toolbar on whether this is +1, 0 or -1 type thing. For me, I'm very used to pip install -r requirements_dev.txt. I suspect I will get tripped up by this change for a while. That said, I'm a 0 on it. I like the docs dependency group for read the docs.
|
I think dependency groups is the way to go (except if everybody starts using something like uv projects). I worry slightly that e.g. the system pip on Ubuntu 22.04 (22.0.2, shipping with Python 3.10) doesn't support dependency groups yet. When using virtualenvs the first thing I do is update pip anyway so it's not a big concern, but I wonder. That said, people are probably (and should be) using newer versions of the OS and Python in their development environments for current projects. We don't support outdated versions of Python and Django, and Django also does not. So we're probably fine moving ahead with this. Re. readthedocs, I really can't say. I've mostly been using old techniques as well when I had to install requirements. We will see what happens. |
9191aea to
75da9e9
Compare
75da9e9 to
422a6b1
Compare
This reverts commit 31d7fb1.
e911892 to
6af4bcc
Compare
|
Rebased on main. Will squash these commits down, then wrangle with ReadTheDocs. |
|
Thank you @p-r-a-v-i-n! |
since we are using pyproject.toml depending on extra file for dependency is redundant. we should use the power of pyproject.toml to take care of it.
according PEP 735 we can use dependency groups which is more convenient.