This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Add **kwargs to reinitialize_command
Type: enhancement Stage: resolved
Components: Distutils2 Versions: Python 3.3, 3rd party
process
Status: closed Resolution: out of date
Dependencies: Superseder:
Assigned To: eric.araujo Nosy List: alexis, eric.araujo, higery, tarek, thomas.holmes
Priority: high Keywords: patch

Created on 2011-06-16 02:08 by higery, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
6b68c2835d6e.diff higery, 2011-06-17 01:34 review
d863f392c094.diff thomas.holmes, 2011-07-09 03:21 review
115c0c10b3ba.diff thomas.holmes, 2011-07-12 01:43 review
7db732ac6796.diff thomas.holmes, 2011-10-12 16:54 review
1bb1132840e6.diff thomas.holmes, 2011-10-12 17:25 review
d2-get_reinitialized_command-kwargs.diff eric.araujo, 2011-10-13 16:04 review
fix-reinitialize-command.diff eric.araujo, 2011-11-12 14:11 review
rename-grc.diff eric.araujo, 2011-11-12 14:14
Repositories containing patches
https://bitbucket.org/higery/cpython#add-keywards
https://bitbucket.org/thomas.holmes/cpython#12344-packaging
Messages (22)
msg138409 - (view) Author: higery (higery) Date: 2011-06-16 02:08
There is a 'reinitialize_command' function in setuptools' command class which can initialize a command with a key-value pair, but it seems that distutils2/packaging does not yet have this function. I think it's useful in the condition that we want to run some commands with some options initialized. 

For instance,  if we want to run 'build_ext' command with its 'inplace' option initialized, we can use : self.reinitialize_command('build_ext', inplace=1), and then self.run_command('build_ext') .
msg138438 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-06-16 14:10
As I told in another message, we have renamed the function to get_reinitialized_command, to make it consistent with other methods’ names.  Instead of adding a function, can you update the existing one?

About the repository: You should not put the user in the URI.  Can you fix it?  The “Create Patch” button doesn’t worK.
msg138453 - (view) Author: higery (higery) Date: 2011-06-16 15:28
But can the get_reinitialized_command function reinitialize a command with a key-value pair? e.g. reinitialize_command('build_ext', inplace=1)

BTW,how to set the repository URI?
msg138454 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-06-16 15:32
> But can the get_reinitialized_command function reinitialize a command
> with a key-value pair?
No, that’s why I did not close this report but asked you if you could make a patch to update this method.

> BTW,how to set the repository URI?
If there is no way to edit the repo, then just add another one.
msg138459 - (view) Author: higery (higery) Date: 2011-06-16 16:15
>No, that’s why I did not close this report but asked you if you could make a patch to update this method.

OK, I think I can try to fix it later.
msg138486 - (view) Author: higery (higery) Date: 2011-06-17 01:54
Not yet tested.
msg140023 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-07-08 11:39
I don’t know if you’ve seen the review I made (there is a bug with user names on the code review site, sometimes mail does not get sent).
msg140048 - (view) Author: Thomas Holmes (thomas.holmes) Date: 2011-07-09 03:21
I have made a patch based on your suggestions made to higery, Éric. I have two questions:

1) In Distributions.get_reinitialized_command should the reinitialization of the subcommands also get passed the kwargs? Unfortunately my understanding of the (sub)command flow is not rock solid.

2) What are your thoughts on an effective test for this? This command does not currently have one. I believe I know a simple way to test the new kwargs functionality but if one were to write a test for get_reinitialized_command I think it should probably test the rest of the function as well.

Thoughts?
msg140122 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-07-11 13:12
> In Distributions.get_reinitialized_command should the
> reinitialization of the subcommands also get passed the kwargs?
Yes, the kwargs need to be passed all the way.

> Unfortunately my understanding of the (sub)command flow is not rock
> solid.
Some methods on the command classes are just convenience wrappers for methods on the distribution object.  That’s why I propose here that the real work is done in Distribution.get_reinitialized_command, and that Command.get_reinitialized_command just delegates.

> What are your thoughts on an effective test for this?
Create a command obj with some options.  Call get_reinitialized_command.  Check that its options have the default values, not the values set on the first command obj.

Do the same thing a second time, this time with some kwargs.  Check they’re set on the resulting command obj.

See also three comments about style on http://bugs.python.org/review/8668/diff2/2845:3011/7845
msg140125 - (view) Author: Thomas Holmes (thomas.holmes) Date: 2011-07-11 14:12
> See also three comments about style on http://bugs.python.org/review/8668/diff2/2845:3011/7845

I'm not sure I follow? The patch I attached does not violate the style guidelines that you indicate in the comments. The only failing I see is that get_reinitialized_command inside cmd.py does not have a doc string. I was following the path of minimal change, but I will be happy to add one.


Unless I am misunderstanding something about the other style comments I will work up another version with subcommand kwargs, docstring, and tests.

Thanks again Éric
msg140130 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-07-11 15:29
> The patch I attached does not violate the style guidelines that you
> indicate in the comments.

Then it’s good.  I was hurrying for my train, so I didn’t check your patch but wanted to reference my comments somewhere before I forgot them.
msg140168 - (view) Author: Thomas Holmes (thomas.holmes) Date: 2011-07-12 01:41
I have added a test and made some additional modifications. I had to modify the very simple MyCmd object in test_command_cmd.py so that it would have some user options with default values and support finalization.
msg144293 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-09-19 15:58
I’d like to commit this; do you need help to address the review I did, or do you want me to take over if you don’t have the time?
msg144297 - (view) Author: Thomas Holmes (thomas.holmes) Date: 2011-09-19 16:20
I got a bit distracted and I think had some questions on the review. I will go back over the state of this patch and get some feedback to you tonight or tomorrow evening.
msg145419 - (view) Author: Thomas Holmes (thomas.holmes) Date: 2011-10-12 17:26
Still working with Éric to determine the proper behavior of get_reinitialized_commands. Latest patch is known incorrect but a step closer to our destination.
msg145466 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-10-13 16:04
This patch has tests that look sensible and pass.
msg145545 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-10-14 16:11
BTW, sorry I did not make a patch that could be applied on top of your repo; I intended to, but your patch did not apply cleanly, so when merging conflicts I also did other editions and then it was too late.

I hope you appreciate how I solved the issue of going twice through kwargs.items(): The code is here twice, but only one of these code paths will be followed in each call :)
msg145579 - (view) Author: Thomas Holmes (thomas.holmes) Date: 2011-10-15 01:44
It's quite alright. Thanks for finishing it up. I realize I ended up a by out of my depth with regards to packaging's internals and the nuance involved in this fix. Thanks for all the help.
msg145702 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-10-17 14:39
No, thank you for contributing :)  I don’t know all internals by heart either, I just needed to step back from the patch and look hard at the tests to make them look good.

I didn’t say that the patch was finished by the way, I haven’t covered all combinations (getting a command already initialized or getting a never-created-before command, giving kwargs or not, what we discussed on IRC).
msg147486 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-11-12 14:11
I’ve spent some time on this.

First, I decided that the former name (reinitialize_command) of the method was better.  The get_* name could create the impression that the returned object was independent from the internal caches (command_obj, have_run), but it was not.  I changed the name back (not pushed yet; I can’t push today).

Second, I wrote tests before adding **kwargs.  It turns out I ended up removing two buggy lines in dist.py!  Thomas, if you could review that patch to tell me 1) if you can understand what it’s doing 2) if removing the two lines was right (they’ve been here since the first commit; on one hand there were no tests, but on the other there were commands depending on that code that did work well), that would be a great help.  You’ve dug into that code, so if you can’t follow my patch it’d be a sign that it needs more comments.

Finally, adding **kwargs was a two-line change in dist.py, a cleanup in some commands to use it, and two more tests.

BTW, some things I said were stupid:
>> In Distributions.get_reinitialized_command should the
>> reinitialization of the subcommands also get passed the kwargs?
> Yes, the kwargs need to be passed all the way.
That made no sense.  Subcommands don’t have the same options as their parent.  The kwargs are only for the command given as argument; to edit the options of a subcommand, reinitialize_command needs to be called for that subcommand too.

>> Unfortunately my understanding of the (sub)command flow is not rock solid.
My reply to that was off-mark.  Documenting how subcommands work is on my todo list, and I’ll be glad to have your feedback on that if you have time.
msg147487 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-11-12 14:14
BTW, here’s the changeset to rename get_reinit_etc.
msg147673 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-11-15 14:23
I’ve noticed that setuptools’ Command class also allows keyword arguments in its constructor.  I’m not sure if it would be useful, but I’ve not looked in depth at the packaging codebase to see if there is code that could use that.
History
Date User Action Args
2022-04-11 14:57:18adminsetgithub: 56553
2014-03-12 10:52:53eric.araujosetstatus: open -> closed
resolution: out of date
stage: patch review -> resolved
2012-02-27 12:23:33eric.araujounlinkissue8668 dependencies
2011-11-15 14:23:18eric.araujosetmessages: + msg147673
2011-11-12 14:14:34eric.araujosetfiles: + rename-grc.diff

messages: + msg147487
2011-11-12 14:11:46eric.araujosetfiles: + fix-reinitialize-command.diff
2011-11-12 14:11:33eric.araujosettitle: Add **kwargs to get_reinitialized_command -> Add **kwargs to reinitialize_command
stage: patch review
messages: + msg147486
versions: + 3rd party
2011-10-17 14:39:41eric.araujosetmessages: + msg145702
2011-10-15 01:44:24thomas.holmessetmessages: + msg145579
2011-10-14 16:11:21eric.araujosetmessages: + msg145545
2011-10-13 16:04:10eric.araujosetfiles: + d2-get_reinitialized_command-kwargs.diff

messages: + msg145466
2011-10-12 17:26:13thomas.holmessetmessages: + msg145419
2011-10-12 17:25:01thomas.holmessetfiles: + 1bb1132840e6.diff
2011-10-12 16:54:12thomas.holmessetfiles: + 7db732ac6796.diff
2011-10-06 14:57:28eric.araujosetpriority: normal -> high
2011-09-19 16:20:31thomas.holmessetmessages: + msg144297
2011-09-19 15:58:24eric.araujosetmessages: + msg144293
2011-07-22 01:10:45eric.araujolinkissue12302 dependencies
2011-07-12 01:43:26thomas.holmessetfiles: + 115c0c10b3ba.diff
2011-07-12 01:43:08thomas.holmessetfiles: - 7c61eba07f3f.diff
2011-07-12 01:41:36thomas.holmessetfiles: + 7c61eba07f3f.diff
2011-07-12 01:41:01thomas.holmessetmessages: + msg140168
2011-07-11 15:29:57eric.araujosetmessages: + msg140130
2011-07-11 14:12:10thomas.holmessetmessages: + msg140125
2011-07-11 13:12:57eric.araujosetmessages: + msg140122
2011-07-09 03:21:34thomas.holmessetfiles: + d863f392c094.diff
2011-07-09 03:21:03thomas.holmessethgrepos: + hgrepo39

messages: + msg140048
nosy: + thomas.holmes
2011-07-08 11:39:12eric.araujosetmessages: + msg140023
2011-06-17 01:54:24higerysetmessages: + msg138486
2011-06-17 01:34:35higerysetfiles: + 6b68c2835d6e.diff
keywords: + patch
2011-06-16 16:16:17eric.araujolinkissue8668 dependencies
2011-06-16 16:15:45higerysetmessages: + msg138459
2011-06-16 15:32:48eric.araujosetassignee: tarek -> eric.araujo
messages: + msg138454
title: A kind of 'reinitialize_command' function which can initialize a command with key-value pair should be added for Command class -> Add **kwargs to get_reinitialized_command
2011-06-16 15:28:29higerysetmessages: + msg138453
2011-06-16 14:10:23eric.araujosetmessages: + msg138438
2011-06-16 08:24:49higerysethgrepos: + hgrepo28
2011-06-16 02:08:25higerycreate