Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Switch to a pip-tools workflow #67
Switch to a pip-tools workflow #67
Changes from all commits
31e07b7
b85f214
46cf74b
c4a9e9a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
This file was deleted.
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.
As the commands are this short, I think it's ok to omit the scripts. The scripts are also not windows compatible (sans WSL), which it would be nice to maintain 1st-party support for.
Also, currently the scripts imply that the virtualenv has been activated, which isn't explicitly documented here, so we should probably document that!
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.
If there was tab completion on
mkdocs
I'd agree about omitting the scripts.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 was actually wondering about including windows implementations for the scripts, so that if we happen to change the build setup again we could do so without needing to change what contributors need to run.
I've mixed feelings about documenting that the virtualenv needs to be active. We weren't doing do before and you don't seem to be advocating to do so for running the
mkdocs
commands directly. I'm therefore not sure why the scripts should be treated any differently.In any of these cases (including the previous setup) the reader still needed to manage their environment themselves to ensure that the command being run was available. Nothing has changed in that regard.
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.
Including windows scripts could also work, but it's a lot of extra scripting. Having the scripts auto-source the virtualenv would definitely make them more useful in my eyes.
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.
How do you propose that they do that? They won't know where the virtualenv is, nor even if one is needed. Setup of the virtualenv is up to the developer at the moment (this has not changed).
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 entire thread feels like we are reimplementing
pipenv
using an less standard tool. I'd personally rather we used a PEP 518 compliant tool, rather than yet another non-standard one.Please could you give some specific examples of where this has caused an issue with this repo? I dislike
pipenv
, but it is probably the best tool for this project.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.
https://studentrobotics.slack.com/archives/CEG05AAG6/p1567264146004600
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.
Okay, I agree that this is an issue.
However, I still don't think that we should be working around the limitations of
pip-tools
by hacking virtualenv activation into scripts. If that is a required feature of the toolset, I suggest we use a tool that does it. Alternatively, we could use pip-tools and provide instructions on how to use a virtualenv /pip-tools
. A link to the appropriate docs is sufficient imo.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.
Well, sort-of. In reality
pipenv
isn't going to be installed for most people by default, either in their host system or within a virtualenv they create. As a result, they end up needing to either install it in this virtualenv (which means they need to activate this virtualenv first anyway) or learn the mastery of having it in another virtualenv without that virtualenv being fully active when they run things (yet havingpipenv
onPATH
). The former of these haspipenv
just be a complication over the regular python tools (i.e:pip
) and the latter requires substantial Python & system admin-fu which it seems unreasonable to expect from casual contributors.If we want to take the opinion that this repo wants to control the virtualenv which it runs in, then we should provide scripts and documentation around creating that, rather than assuming that the user knows how to do so. Until now, it hasn't done that. (If we did do this I would argue we should still allow the user to have their environment somewhere else if they wanted, probably by having our scripts detect an existing env is active, so that people can use things like
virtualenvwrapper
if they want)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 thread has gone to quite a tangent now, and isn't especially constructive to the PR.
I'd not noticed the documented setup instructions don't mention a virtualenv. I can live with the scripts staying there, and as the platform-agnostic commands are documented first, that should be fine.
I'd be interested in simply documenting that virtualenvs exist, and are considered best practice, so volunteers don't pollute their global environment.