| Message ID | CAHfxMJ4cFRmUBteM-tM_gM=5fbdupjJRaahnbc34wifYgSn=gg@mail.gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] utils/scanpypi: supply package name to setup() when not provided | expand |
Eric, All, +James for his expertise in that file On 2022-09-12 09:28 -0700, erichiggins@gmail.com spake thusly: > Signed-off-by: Eric Higgins <erichiggins@gmail.com> Thanks for this patch. However, this will need a bit more explanations in the commit log. Start by describing the issue, explain why that happens, and how it is fixed. You can get an idea of how to structure that by looking at existing commit logs: git log utils/scanpypi > --- > utils/scanpypi | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/utils/scanpypi b/utils/scanpypi > index 452b4a3fc3..a5522a879e 100755 > --- a/utils/scanpypi > +++ b/utils/scanpypi > @@ -58,8 +58,9 @@ def setup_decorator(func, method): > def closure(*args, **kwargs): > # Any python packages calls its setup function to be installed. > # Argument 'name' of this setup function is the package's name So, this comment states that setup() is called with 'name' argument, but what your commit title implies is that it is not always true. So, this comment is now incorrect, and must be amended apropriately. Could it be that sometimes, 'name' is a keyword argument, and in some other case, it is just a positional argument? > - BuildrootPackage.setup_args[kwargs['name']] = kwargs > - BuildrootPackage.setup_args[kwargs['name']]['method'] = method > + name = kwargs.get('name', BuildrootPackage.setup_args['name']) > + BuildrootPackage.setup_args[name] = kwargs > + BuildrootPackage.setup_args[name]['method'] = method > return closure > > # monkey patch > @@ -147,6 +148,7 @@ class BuildrootPackage(): > self.url = None > self.version = None > self.license_files = [] > + self.setup_args['name'] = self.real_name Otherwise, I do understand what the code does, and I think this is the correct solution. James, your opinion?. Still, what is missing is an explanation on why this change is needed. Regards, Yann E. MORIN. > def fetch_package_info(self): > """ > -- > 2.25.1 > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot
Yann, Eric, On 12.09.22 22:34, Yann E. MORIN wrote: > Eric, All, > > +James for his expertise in that file > > On 2022-09-12 09:28 -0700, erichiggins@gmail.com spake thusly: >> Signed-off-by: Eric Higgins <erichiggins@gmail.com> > > Thanks for this patch. > > However, this will need a bit more explanations in the commit log. Start > by describing the issue, explain why that happens, and how it is fixed. > > You can get an idea of how to structure that by looking at existing > commit logs: git log utils/scanpypi > >> --- >> utils/scanpypi | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/utils/scanpypi b/utils/scanpypi >> index 452b4a3fc3..a5522a879e 100755 >> --- a/utils/scanpypi >> +++ b/utils/scanpypi >> @@ -58,8 +58,9 @@ def setup_decorator(func, method): >> def closure(*args, **kwargs): >> # Any python packages calls its setup function to be installed. >> # Argument 'name' of this setup function is the package's name > > So, this comment states that setup() is called with 'name' argument, but > what your commit title implies is that it is not always true. So, this > comment is now incorrect, and must be amended apropriately. > > Could it be that sometimes, 'name' is a keyword argument, and in some > other case, it is just a positional argument? I can offer an example of where the existing script goes wrong: https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.py No arguments passed at all. `name` (like anything else) in this case is read from the accompanying setup.cfg file (in medium-modern python packaging world): https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.cfg#L2 This is explained for example here: https://towardsdatascience.com/setuptools-python-571e7d5500f2 In the even more modern world the same info is specified in pyproject.toml instead: https://github.com/agronholm/apscheduler/blob/49344e6954559259beb336436a45698d62eed5b4/pyproject.toml#L2-L9 But I think it's easiest and correct to use the name specified on the cli instead for us. > >> - BuildrootPackage.setup_args[kwargs['name']] = kwargs >> - BuildrootPackage.setup_args[kwargs['name']]['method'] = method >> + name = kwargs.get('name', BuildrootPackage.setup_args['name']) >> + BuildrootPackage.setup_args[name] = kwargs >> + BuildrootPackage.setup_args[name]['method'] = method >> return closure >> >> # monkey patch >> @@ -147,6 +148,7 @@ class BuildrootPackage(): >> self.url = None >> self.version = None >> self.license_files = [] >> + self.setup_args['name'] = self.real_name > > Otherwise, I do understand what the code does, and I think this is the > correct solution. James, your opinion?. > > Still, what is missing is an explanation on why this change is needed. > > Regards, > Yann E. MORIN. > >> def fetch_package_info(self): >> """ >> -- >> 2.25.1 >> _______________________________________________ >> buildroot mailing list >> buildroot@buildroot.org >> https://lists.buildroot.org/mailman/listinfo/buildroot >
Yann, I did a write up w/ the justification for this change in this Github gist https://gist.github.com/erichiggins/223b2495ada64001c44deedd3a3df6ed Hopefully that provides the necessary info, but I'm happy to copy/paste it here if you need it for the mailing list record. On Mon, Sep 12, 2022 at 2:05 PM Marcus Hoffmann <marcus.hoffmann@othermo.de> wrote: > Yann, Eric, > > On 12.09.22 22:34, Yann E. MORIN wrote: > > Eric, All, > > > > +James for his expertise in that file > > > > On 2022-09-12 09:28 -0700, erichiggins@gmail.com spake thusly: > >> Signed-off-by: Eric Higgins <erichiggins@gmail.com> > > > > Thanks for this patch. > > > > However, this will need a bit more explanations in the commit log. Start > > by describing the issue, explain why that happens, and how it is fixed. > > > > You can get an idea of how to structure that by looking at existing > > commit logs: git log utils/scanpypi > > > >> --- > >> utils/scanpypi | 6 ++++-- > >> 1 file changed, 4 insertions(+), 2 deletions(-) > >> > >> diff --git a/utils/scanpypi b/utils/scanpypi > >> index 452b4a3fc3..a5522a879e 100755 > >> --- a/utils/scanpypi > >> +++ b/utils/scanpypi > >> @@ -58,8 +58,9 @@ def setup_decorator(func, method): > >> def closure(*args, **kwargs): > >> # Any python packages calls its setup function to be > installed. > >> # Argument 'name' of this setup function is the package's name > > > > So, this comment states that setup() is called with 'name' argument, but > > what your commit title implies is that it is not always true. So, this > > comment is now incorrect, and must be amended apropriately. > > > > Could it be that sometimes, 'name' is a keyword argument, and in some > > other case, it is just a positional argument? > > I can offer an example of where the existing script goes wrong: > > > https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.py > > No arguments passed at all. `name` (like anything else) in this case is > read from the accompanying setup.cfg file (in medium-modern python > packaging world): > > > https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.cfg#L2 > > This is explained for example here: > https://towardsdatascience.com/setuptools-python-571e7d5500f2 > > In the even more modern world the same info is specified in > pyproject.toml instead: > > > https://github.com/agronholm/apscheduler/blob/49344e6954559259beb336436a45698d62eed5b4/pyproject.toml#L2-L9 > > But I think it's easiest and correct to use the name specified on the > cli instead for us. > > > > >> - BuildrootPackage.setup_args[kwargs['name']] = kwargs > >> - BuildrootPackage.setup_args[kwargs['name']]['method'] = method > >> + name = kwargs.get('name', BuildrootPackage.setup_args['name']) > >> + BuildrootPackage.setup_args[name] = kwargs > >> + BuildrootPackage.setup_args[name]['method'] = method > >> return closure > >> > >> # monkey patch > >> @@ -147,6 +148,7 @@ class BuildrootPackage(): > >> self.url = None > >> self.version = None > >> self.license_files = [] > >> + self.setup_args['name'] = self.real_name > > > > Otherwise, I do understand what the code does, and I think this is the > > correct solution. James, your opinion?. > > > > Still, what is missing is an explanation on why this change is needed. > > > Regards, > > Yann E. MORIN. > > > >> def fetch_package_info(self): > >> """ > >> -- > >> 2.25.1 > >> _______________________________________________ > >> buildroot mailing list > >> buildroot@buildroot.org > >> https://lists.buildroot.org/mailman/listinfo/buildroot > > >
Eric, All, On 2022-09-12 14:34 -0700, erichiggins@gmail.com spake thusly: > Yann, > I did a write up w/ the justification for this change in this Github gist > [1]https://gist.github.com/erichiggins/223b2495ada64001c44deedd3a3df6ed > Hopefully that provides the necessary info, but I'm happy to copy/paste it here if you need it for the mailing list record. Please, resubmit this change with the appropriate explanations from your gist if that makes sense, reformatted as a proper commit log; 1. describe the issue 2. explain why it happens 3. explain how you fixed it (don't describe the code, explain it). Regards, Yann E. MORIN. > On Mon, Sep 12, 2022 at 2:05 PM Marcus Hoffmann < [2]marcus.hoffmann@othermo.de> wrote: > > Yann, Eric, > > On 12.09.22 22:34, Yann E. MORIN wrote: > > Eric, All, > > > > +James for his expertise in that file > > > > On 2022-09-12 09:28 -0700, [3]erichiggins@gmail.com spake thusly: > >> Signed-off-by: Eric Higgins < [4]erichiggins@gmail.com> > > > > Thanks for this patch. > > > > However, this will need a bit more explanations in the commit log. Start > > by describing the issue, explain why that happens, and how it is fixed. > > > > You can get an idea of how to structure that by looking at existing > > commit logs: git log utils/scanpypi > > > >> --- > >> utils/scanpypi | 6 ++++-- > >> 1 file changed, 4 insertions(+), 2 deletions(-) > >> > >> diff --git a/utils/scanpypi b/utils/scanpypi > >> index 452b4a3fc3..a5522a879e 100755 > >> --- a/utils/scanpypi > >> +++ b/utils/scanpypi > >> @@ -58,8 +58,9 @@ def setup_decorator(func, method): > >> def closure(*args, **kwargs): > >> # Any python packages calls its setup function to be installed. > >> # Argument 'name' of this setup function is the package's name > > > > So, this comment states that setup() is called with 'name' argument, but > > what your commit title implies is that it is not always true. So, this > > comment is now incorrect, and must be amended apropriately. > > > > Could it be that sometimes, 'name' is a keyword argument, and in some > > other case, it is just a positional argument? > > I can offer an example of where the existing script goes wrong: > > [5]https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.py > > No arguments passed at all. `name` (like anything else) in this case is > read from the accompanying setup.cfg file (in medium-modern python > packaging world): > > [6]https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.cfg#L2 > > This is explained for example here: > [7]https://towardsdatascience.com/setuptools-python-571e7d5500f2 > > In the even more modern world the same info is specified in > pyproject.toml instead: > > [8]https://github.com/agronholm/apscheduler/blob/49344e6954559259beb336436a45698d62eed5b4/pyproject.toml#L2-L9 > > But I think it's easiest and correct to use the name specified on the > cli instead for us. > > > > >> - BuildrootPackage.setup_args[kwargs['name']] = kwargs > >> - BuildrootPackage.setup_args[kwargs['name']]['method'] = method > >> + name = kwargs.get('name', BuildrootPackage.setup_args['name']) > >> + BuildrootPackage.setup_args[name] = kwargs > >> + BuildrootPackage.setup_args[name]['method'] = method > >> return closure > >> > >> # monkey patch > >> @@ -147,6 +148,7 @@ class BuildrootPackage(): > >> self.url = None > >> self.version = None > >> self.license_files = [] > >> + self.setup_args['name'] = self.real_name > > > > Otherwise, I do understand what the code does, and I think this is the > > correct solution. James, your opinion?. > > > > Still, what is missing is an explanation on why this change is needed. > > > Regards, > > Yann E. MORIN. > > > >> def fetch_package_info(self): > >> """ > >> -- > >> 2.25.1 > >> _______________________________________________ > >> buildroot mailing list > >> [9]buildroot@buildroot.org > >> [10]https://lists.buildroot.org/mailman/listinfo/buildroot > > > > Links: > 1. https://gist.github.com/erichiggins/223b2495ada64001c44deedd3a3df6ed > 2. mailto:marcus.hoffmann@othermo.de > 3. mailto:erichiggins@gmail.com > 4. mailto:erichiggins@gmail.com > 5. https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.py > 6. https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.cfg#L2 > 7. https://towardsdatascience.com/setuptools-python-571e7d5500f2 > 8. https://github.com/agronholm/apscheduler/blob/49344e6954559259beb336436a45698d62eed5b4/pyproject.toml#L2-L9 > 9. mailto:buildroot@buildroot.org > 10. https://lists.buildroot.org/mailman/listinfo/buildroot > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot
🙄 fine. On Sun, Sep 18, 2022 at 9:00 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > Eric, All, > > On 2022-09-12 14:34 -0700, erichiggins@gmail.com spake thusly: > > Yann, > > I did a write up w/ the justification for this change in this Github gist > > [1]https://gist.github.com/erichiggins/223b2495ada64001c44deedd3a3df6ed > > Hopefully that provides the necessary info, but I'm happy to copy/paste it here if you need it for the mailing list record. > > Please, resubmit this change with the appropriate explanations from your > gist if that makes sense, reformatted as a proper commit log; > > 1. describe the issue > 2. explain why it happens > 3. explain how you fixed it (don't describe the code, explain it). > > Regards, > Yann E. MORIN. > > > On Mon, Sep 12, 2022 at 2:05 PM Marcus Hoffmann < [2]marcus.hoffmann@othermo.de> wrote: > > > > Yann, Eric, > > > > On 12.09.22 22:34, Yann E. MORIN wrote: > > > Eric, All, > > > > > > +James for his expertise in that file > > > > > > On 2022-09-12 09:28 -0700, [3]erichiggins@gmail.com spake thusly: > > >> Signed-off-by: Eric Higgins < [4]erichiggins@gmail.com> > > > > > > Thanks for this patch. > > > > > > However, this will need a bit more explanations in the commit log. Start > > > by describing the issue, explain why that happens, and how it is fixed. > > > > > > You can get an idea of how to structure that by looking at existing > > > commit logs: git log utils/scanpypi > > > > > >> --- > > >> utils/scanpypi | 6 ++++-- > > >> 1 file changed, 4 insertions(+), 2 deletions(-) > > >> > > >> diff --git a/utils/scanpypi b/utils/scanpypi > > >> index 452b4a3fc3..a5522a879e 100755 > > >> --- a/utils/scanpypi > > >> +++ b/utils/scanpypi > > >> @@ -58,8 +58,9 @@ def setup_decorator(func, method): > > >> def closure(*args, **kwargs): > > >> # Any python packages calls its setup function to be installed. > > >> # Argument 'name' of this setup function is the package's name > > > > > > So, this comment states that setup() is called with 'name' argument, but > > > what your commit title implies is that it is not always true. So, this > > > comment is now incorrect, and must be amended apropriately. > > > > > > Could it be that sometimes, 'name' is a keyword argument, and in some > > > other case, it is just a positional argument? > > > > I can offer an example of where the existing script goes wrong: > > > > [5]https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.py > > > > No arguments passed at all. `name` (like anything else) in this case is > > read from the accompanying setup.cfg file (in medium-modern python > > packaging world): > > > > [6]https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.cfg#L2 > > > > This is explained for example here: > > [7]https://towardsdatascience.com/setuptools-python-571e7d5500f2 > > > > In the even more modern world the same info is specified in > > pyproject.toml instead: > > > > [8]https://github.com/agronholm/apscheduler/blob/49344e6954559259beb336436a45698d62eed5b4/pyproject.toml#L2-L9 > > > > But I think it's easiest and correct to use the name specified on the > > cli instead for us. > > > > > > > >> - BuildrootPackage.setup_args[kwargs['name']] = kwargs > > >> - BuildrootPackage.setup_args[kwargs['name']]['method'] = method > > >> + name = kwargs.get('name', BuildrootPackage.setup_args['name']) > > >> + BuildrootPackage.setup_args[name] = kwargs > > >> + BuildrootPackage.setup_args[name]['method'] = method > > >> return closure > > >> > > >> # monkey patch > > >> @@ -147,6 +148,7 @@ class BuildrootPackage(): > > >> self.url = None > > >> self.version = None > > >> self.license_files = [] > > >> + self.setup_args['name'] = self.real_name > > > > > > Otherwise, I do understand what the code does, and I think this is the > > > correct solution. James, your opinion?. > > > > > > Still, what is missing is an explanation on why this change is needed. > > > > Regards, > > > Yann E. MORIN. > > > > > >> def fetch_package_info(self): > > >> """ > > >> -- > > >> 2.25.1 > > >> _______________________________________________ > > >> buildroot mailing list > > >> [9]buildroot@buildroot.org > > >> [10]https://lists.buildroot.org/mailman/listinfo/buildroot > > > > > > > Links: > > 1. https://gist.github.com/erichiggins/223b2495ada64001c44deedd3a3df6ed > > 2. mailto:marcus.hoffmann@othermo.de > > 3. mailto:erichiggins@gmail.com > > 4. mailto:erichiggins@gmail.com > > 5. https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.py > > 6. https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.cfg#L2 > > 7. https://towardsdatascience.com/setuptools-python-571e7d5500f2 > > 8. https://github.com/agronholm/apscheduler/blob/49344e6954559259beb336436a45698d62eed5b4/pyproject.toml#L2-L9 > > 9. mailto:buildroot@buildroot.org > > 10. https://lists.buildroot.org/mailman/listinfo/buildroot > > > _______________________________________________ > > buildroot mailing list > > buildroot@buildroot.org > > https://lists.buildroot.org/mailman/listinfo/buildroot > > > -- > .-----------------.--------------------.------------------.--------------------. > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | > | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | > '------------------------------^-------^------------------^--------------------'
diff --git a/utils/scanpypi b/utils/scanpypi index 452b4a3fc3..a5522a879e 100755 --- a/utils/scanpypi +++ b/utils/scanpypi @@ -58,8 +58,9 @@ def setup_decorator(func, method): def closure(*args, **kwargs): # Any python packages calls its setup function to be installed. # Argument 'name' of this setup function is the package's name - BuildrootPackage.setup_args[kwargs['name']] = kwargs - BuildrootPackage.setup_args[kwargs['name']]['method'] = method + name = kwargs.get('name', BuildrootPackage.setup_args['name']) + BuildrootPackage.setup_args[name] = kwargs + BuildrootPackage.setup_args[name]['method'] = method return closure # monkey patch @@ -147,6 +148,7 @@ class BuildrootPackage(): self.url = None self.version = None self.license_files = [] + self.setup_args['name'] = self.real_name def fetch_package_info(self): """
Signed-off-by: Eric Higgins <erichiggins@gmail.com> --- utils/scanpypi | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)