Add --install-options and --global-options to requirement parser + refactor #790
Conversation
|
wow, fast turnaround, and new tests too. |
| if self.editable: | ||
| self.install_editable(install_options, global_options) | ||
| return | ||
|
|
||
| global_options += self.options.get('global_options', []) | ||
| install_options += self.options.get('install_options', []) |
There was a problem hiding this comment.
what happens when there's dupes or conflicts between the per-line options and the cli options?
this looks to be just sticking them together? is there some override logic somewhere I'm not seeing?
There was a problem hiding this comment.
No, there's no explicit override logic. The last option given on the command line is the one that will be used. In our case requiremens.txt options will always override cmd options. Maybe I should reverse that?
There was a problem hiding this comment.
my thought was that options in the requirements file should override.
that's going to be wanted more often I think... i.e. to add options in the req file for one troublesome requirement that should override general options in the cli.
worried it will somehow break down like this though. we're sure setuptools follows "last option given on the command line is the one that will be used" in all cases?
There was a problem hiding this comment.
Yes, it makes sense in this case, even though command line arguments should typically trump all other sources.
It seems that last argument always wins (experimentally and from looking at the code). I'll add a comment to clarify this.
|
This branch has become unmergable after the work that went in to the wheel command. Rebasing and force pushing with develop 47edb6d. |
|
added this tentatively to the "1.5" milestone. |
|
btw, I never forgot about this PR. I think my block on this, was that I knew I wanted to break up req.py into a package right after merging it, and get your new module under that (and possibly restructure some of your code into a new RequirementsFile class). I would handle bringing this up to date and resolving conflicts, while maintaining your original commits. lotta people want this, so I get that's it important. |
btw, the req.py breakup has been done, so getting this mergeable again will require a little work, and it might be hard to do with a merge and trying to maintain the original commits. |
|
@gvalkov |
|
hi @qwcode, is there any way I can help you on this? I'd really like to see it working soon. |
|
Too bad this had a bunch of work and then slipped through the cracks. I wonder if it could be resurrected. |
|
Is the refactor necessary as part of this change? It would probably be easier to review if the refactor (code moves, no functionality change) and the new feature (functionality change but no code moves) were separate PRs. |
|
I think author's comment Reviewing by commit might be easier than going to 'Files Changed', imho. addresses exactly this concern. |
|
@piotr-dobrogost Not really, it means "reviewing the whole PR may be hard" :-) If reviewing by commit is recommended, then a series of PRs that depend on each other may be more likely to get merged. Anyhow, if I get time I'll review this, all I was saying is that it's less likely I'll get time to review the combined PR. (Personally, I don't find the github UI for reviewing by commit to be particularly easy to use, but that's just me). |
|
Hello @pfmoore. Please hold that review for now. I'll start a new PR with a clean re-implementation of this feature, sans the extra refactoring. |
|
@gvalkov Fantastic, thanks. I appreciate you putting in the extra work. |
|
@gvalkov Thanks I appreciate it too. Can you post the PR here when you have it? |
|
Hello @pfmoore and @msabramo. I just pushed PR #2537 and while I'm still looking into some of the build failures, I think it's ready to be scrutinized. It's a rebase and cleanup of the previous work that was done and as such contains a bit more refactoring than necessary. Please consider #2537 with the |
|
closed via #2537 |
lock
bot
added
the
auto-locked
Hi all,
This pull requests adds the functionality requested in #271 and #515. In brief, it allows you to pass
--install-optionsand--global-optionsin the requirements file. Example:INITools==2.0 --global-options="--help" --install-options="--prefix=/opt"I've also completely refactored
parse_requirements()and added more tests.Reviewing by commit might be easier than going to 'Files Changed', imho.