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.

Sign up for GitHub

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

Jump to bottom

Make Requirements.txt Installation Optional #55

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

Make Requirements.txt Installation Optional #55

wants to merge 2 commits into from

Conversation

frinzekt
Copy link
Contributor

@frinzekt frinzekt commented Dec 5, 2020 •
edited
Loading

To address #53

@mhausenblas
Copy link
Owner

Thanks @frinzekt and @janoszen WDYT?

@ghost
Copy link

ghost commented Dec 7, 2020

This change would mean breaking the installation for everyone who is relying on requirements.txt being used as-is (mine too). I would recommend not breaking people's setup any more than necessary. If this behavior needs to be disabled let's add a flag to disable it.

@mhausenblas
Copy link
Owner

Right @janoszen my thinking as well. I wonder what would be the best DX for it?

@ghost
Copy link

ghost commented Dec 7, 2020

I'm thinking DISABLE_REQUIREMENTS_TXT=1

@frinzekt
Copy link
Contributor Author

frinzekt commented Dec 7, 2020

I agree with everything mentioned here. It's not good to introduce breaking change. Let's just go with @janoszen suggestion about a flag to disable the default behaviour.

@ghost
Copy link

ghost commented Dec 7, 2020

One more thing: we need this behavior in the nomaterial branch too, not just master.

@mhausenblas
Copy link
Owner

Given #57 and #60 can we close this PR?

@ghost
Copy link

ghost commented Jan 4, 2021

@mhausenblas no, those PRs only allow adding extra OS packages, they don't disable using requirements.txt. This PR still needs to be changed to not break functionality for existing users.

thekaveman added a commit to cal-itp/benefits that referenced this pull request Jun 8, 2021
@thekaveman
fix(docs): move mkdocs requirements to file
b12d325 
mhausenblas/mkdocs-deploy-gh-pages pip installs the requirements.txt
from the root of the repo unless overridden with another path

there is currently no way to opt-out; this was under discussion in
Dec 2020: mhausenblas/mkdocs-deploy-gh-pages#55

this is problem, we don't want to install requirements from benefits
for mkdocs, so provide the REQUIREMENTS env with our mkdocs dependencies
@thekaveman thekaveman mentioned this pull request Jun 8, 2021
Merged
@gouttegd gouttegd mentioned this pull request Jan 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atlslscsrv-app atlslscsrv-app atlslscsrv-app approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@frinzekt @mhausenblas @atlslscsrv-app