I think this is ready for review, I have tested it with some projects locally.
I'm converting the list of python.install to a dictionary with its indices as keys, that way is easy to handle with the current strict validation (pop_config).
An installation for a requirements file is always created for v1 (with requirements = None), to keep compatibility with finding a requirements file.
PythonEnvironment now has only one entry point to install things (previously install_user_requirements and install_package).
The order of the list is respected when installing packages.
Installations for pip and setup.py, now accept a path, which is relative to the root of the project. For pip this is a special case, as pip needs to differentiate from a regular package and a path, for that we need to prefix ./ to path (except to .).
# Transform to a dict, so it's easy to validate extra keys.
I think it's not a good idea to change the raw_config object. By doing this, it won't be "raw" anymore and it will bring potential confusions.
I understand that the raw_config is the user's YAML parsed into a Python object (without being touched by us).
Also, checking the validate_python_install I think this can be accessed by an index instead of a key without need to modify the original config object.
We can't access by the index because of pop_config, it needs to pop recursively, and we need to keep the track of the index (if index 3 is deleted, the index 4 passes to be 3, we don't want that). I guess I could create a copy.
It's not clear why this can be removed, could you expand on this?
We have https://docs.readthedocs.io/en/latest/settings.html#use-pip-install, but the logic wasn't correct, I mean, I think this was used when we didn't have a config object, and this will change the behavior of the config from the web Install project using setup to use pip instead. So, this will invert that setting, but now we have the two settings, install with pip or setup, so, we can't guess or rely on install_with_setup or whatever to invert the meaning.
I think the logic before the refactor to support v2 was correct and was inverting the behavior if the user has the config from the web marked as install with setup that should work.
I don't think we need the setting, we're not using this in production.
Regarding the database setting, I think we should be using this. As far as I can tell we aren't using that in this PR?
This code looks good to me and I think we can merge it.
Although, I'd like to have another member of the @rtfd/core to review it since its a good amount of code and having more eyes on it would be good.
Also, I left some opinions on things that I don't like too much and suggestions to improve the code but I don't think they are 100% strict to be done right now.
Actually we could just address this later, but just in case.
We could likely use constants for requirements and path, but not worth blocking this PR on.
Can I merge this after today's deploy?
@stsewd I'm +1 on merging, after fixing conflicts.