| Message ID | 20200227000639.9644-10-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | iotests: use python logging | expand |
On 2/26/20 6:06 PM, John Snow wrote: > Repeatable results. run `pylint iotests.py` and you should get a pass. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > tests/qemu-iotests/pylintrc | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > create mode 100644 tests/qemu-iotests/pylintrc > + too-many-branches, > + too-many-public-methods, > \ No newline at end of file Intentional?
On 2/27/20 1:06 AM, John Snow wrote: > Repeatable results. run `pylint iotests.py` and you should get a pass. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > tests/qemu-iotests/pylintrc | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > create mode 100644 tests/qemu-iotests/pylintrc > > diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc > new file mode 100644 > index 0000000000..feed506f75 > --- /dev/null > +++ b/tests/qemu-iotests/pylintrc > @@ -0,0 +1,20 @@ > +[MESSAGES CONTROL] > + > +# Disable the message, report, category or checker with the given id(s). You > +# can either give multiple identifiers separated by comma (,) or put this > +# option multiple times (only on the command line, not in the configuration > +# file where it should appear only once). You can also use "--disable=all" to > +# disable everything first and then reenable specific checks. For example, if > +# you want to run only the similarities checker, you can use "--disable=all > +# --enable=similarities". If you want to run only the classes checker, but have > +# no Warning level messages displayed, use "--disable=all --enable=classes > +# --disable=W". > +disable=invalid-name, > + missing-docstring, > + line-too-long, > + too-many-lines, > + too-few-public-methods, > + too-many-arguments, > + too-many-locals, > + too-many-branches, > + too-many-public-methods, > \ No newline at end of file > Can you run it in one of the CI jobs?
On 2/27/20 9:11 AM, Philippe Mathieu-Daudé wrote: > On 2/27/20 1:06 AM, John Snow wrote: >> Repeatable results. run `pylint iotests.py` and you should get a pass. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> tests/qemu-iotests/pylintrc | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> create mode 100644 tests/qemu-iotests/pylintrc >> >> diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc >> new file mode 100644 >> index 0000000000..feed506f75 >> --- /dev/null >> +++ b/tests/qemu-iotests/pylintrc >> @@ -0,0 +1,20 @@ >> +[MESSAGES CONTROL] >> + >> +# Disable the message, report, category or checker with the given >> id(s). You >> +# can either give multiple identifiers separated by comma (,) or put >> this >> +# option multiple times (only on the command line, not in the >> configuration >> +# file where it should appear only once). You can also use >> "--disable=all" to >> +# disable everything first and then reenable specific checks. For >> example, if >> +# you want to run only the similarities checker, you can use >> "--disable=all >> +# --enable=similarities". If you want to run only the classes >> checker, but have >> +# no Warning level messages displayed, use "--disable=all >> --enable=classes >> +# --disable=W". >> +disable=invalid-name, >> + missing-docstring, >> + line-too-long, >> + too-many-lines, >> + too-few-public-methods, >> + too-many-arguments, >> + too-many-locals, >> + too-many-branches, >> + too-many-public-methods, >> \ No newline at end of file >> > > Can you run it in one of the CI jobs? > Tell me where to add it and I will do so.
John Snow <jsnow@redhat.com> writes: > Repeatable results. run `pylint iotests.py` and you should get a pass. Start your sentences with a capital letter, please. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > tests/qemu-iotests/pylintrc | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > create mode 100644 tests/qemu-iotests/pylintrc > > diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc > new file mode 100644 > index 0000000000..feed506f75 > --- /dev/null > +++ b/tests/qemu-iotests/pylintrc > @@ -0,0 +1,20 @@ > +[MESSAGES CONTROL] > + > +# Disable the message, report, category or checker with the given id(s). You > +# can either give multiple identifiers separated by comma (,) or put this > +# option multiple times (only on the command line, not in the configuration > +# file where it should appear only once). You can also use "--disable=all" to > +# disable everything first and then reenable specific checks. For example, if > +# you want to run only the similarities checker, you can use "--disable=all > +# --enable=similarities". If you want to run only the classes checker, but have > +# no Warning level messages displayed, use "--disable=all --enable=classes > +# --disable=W". > +disable=invalid-name, > + missing-docstring, > + line-too-long, > + too-many-lines, > + too-few-public-methods, > + too-many-arguments, > + too-many-locals, > + too-many-branches, > + too-many-public-methods, > \ No newline at end of file Add the newline, please. German pejorative for the too-many- and too-few- warnings: "Müsli". Implies it's for muesli-knitters / granola-crunchers indulging their orthorexia. missing-docstring is not advisable for libraries. Feels okay here. line-too-long might be worth cleaning up. How many of them do we have now?
On 3/4/20 2:22 AM, Markus Armbruster wrote: > John Snow <jsnow@redhat.com> writes: > >> Repeatable results. run `pylint iotests.py` and you should get a pass. > > Start your sentences with a capital letter, please. > The German complains about the capitalization, but not the sentence fragment. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> tests/qemu-iotests/pylintrc | 20 ++++++++++++++++++++ >> 1 file changed, 20 insertions(+) >> create mode 100644 tests/qemu-iotests/pylintrc >> >> diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc >> new file mode 100644 >> index 0000000000..feed506f75 >> --- /dev/null >> +++ b/tests/qemu-iotests/pylintrc >> @@ -0,0 +1,20 @@ >> +[MESSAGES CONTROL] >> + >> +# Disable the message, report, category or checker with the given id(s). You >> +# can either give multiple identifiers separated by comma (,) or put this >> +# option multiple times (only on the command line, not in the configuration >> +# file where it should appear only once). You can also use "--disable=all" to >> +# disable everything first and then reenable specific checks. For example, if >> +# you want to run only the similarities checker, you can use "--disable=all >> +# --enable=similarities". If you want to run only the classes checker, but have >> +# no Warning level messages displayed, use "--disable=all --enable=classes >> +# --disable=W". >> +disable=invalid-name, >> + missing-docstring, >> + line-too-long, >> + too-many-lines, >> + too-few-public-methods, >> + too-many-arguments, >> + too-many-locals, >> + too-many-branches, >> + too-many-public-methods, >> \ No newline at end of file > > Add the newline, please. > > German pejorative for the too-many- and too-few- warnings: "Müsli". > Implies it's for muesli-knitters / granola-crunchers indulging their > orthorexia. > They are useful at times as they can suggest when you are going a bit overboard on "organically grown" design. For cleaning an existing codebase, it's more of a hindrance to the immediate goal of establishing a baseline. (*cough* I try to adhere to them in my own freshly written code, and disable per-line when I've decided to veto the suggestion. Not appropriate for a codebase like ours. As Max reminds me, it's just tests -- don't make them too clever or pretty.) Regardless. It's not appropriate here and now. > missing-docstring is not advisable for libraries. Feels okay here. > Ideally we do start using them, but it's out of scope here. Since I did some cleanup, I wanted to establish the baseline of what I adhered to. *not* suggest that it's the destination state. Adding proper docstrings should be done during mypy conversion once the types are determined, understood, and enforced. Not before then. > line-too-long might be worth cleaning up. How many of them do we have > now? > Five in iotests.py using the default length of 100. 15 if I limit to 80. If we agree that 100 is okay, I can tackle this in an amendment patch. If 80 is okay, I'm going to put it off as one coat of paint too many. (I will try to clean up the 100+ lines for my next version. I am hesitant to make a deeper cut because I have the feeling it's the type of series that will incur a lot of nitpicks on indent style.) --js
John Snow <jsnow@redhat.com> writes: > On 3/4/20 2:22 AM, Markus Armbruster wrote: >> John Snow <jsnow@redhat.com> writes: >> >>> Repeatable results. run `pylint iotests.py` and you should get a pass. >> >> Start your sentences with a capital letter, please. >> > > The German complains about the capitalization, but not the sentence > fragment. Heh! >>> >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> tests/qemu-iotests/pylintrc | 20 ++++++++++++++++++++ >>> 1 file changed, 20 insertions(+) >>> create mode 100644 tests/qemu-iotests/pylintrc >>> >>> diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc >>> new file mode 100644 >>> index 0000000000..feed506f75 >>> --- /dev/null >>> +++ b/tests/qemu-iotests/pylintrc >>> @@ -0,0 +1,20 @@ >>> +[MESSAGES CONTROL] >>> + >>> +# Disable the message, report, category or checker with the given id(s). You >>> +# can either give multiple identifiers separated by comma (,) or put this >>> +# option multiple times (only on the command line, not in the configuration >>> +# file where it should appear only once). You can also use "--disable=all" to >>> +# disable everything first and then reenable specific checks. For example, if >>> +# you want to run only the similarities checker, you can use "--disable=all >>> +# --enable=similarities". If you want to run only the classes checker, but have >>> +# no Warning level messages displayed, use "--disable=all --enable=classes >>> +# --disable=W". >>> +disable=invalid-name, >>> + missing-docstring, >>> + line-too-long, >>> + too-many-lines, >>> + too-few-public-methods, >>> + too-many-arguments, >>> + too-many-locals, >>> + too-many-branches, >>> + too-many-public-methods, >>> \ No newline at end of file >> >> Add the newline, please. >> >> German pejorative for the too-many- and too-few- warnings: "Müsli". >> Implies it's for muesli-knitters / granola-crunchers indulging their >> orthorexia. >> > > They are useful at times as they can suggest when you are going a bit > overboard on "organically grown" design. For cleaning an existing > codebase, it's more of a hindrance to the immediate goal of establishing > a baseline. Yes, gentle nudges to reconsider your code organization can be useful. But when we run pylint with the goal of getting no output, even warnings are much more than gentle nudges. > (*cough* I try to adhere to them in my own freshly written code, and > disable per-line when I've decided to veto the suggestion. Not > appropriate for a codebase like ours. As Max reminds me, it's just tests > -- don't make them too clever or pretty.) > > Regardless. It's not appropriate here and now. > >> missing-docstring is not advisable for libraries. Feels okay here. >> > > Ideally we do start using them, but it's out of scope here. Since I did > some cleanup, I wanted to establish the baseline of what I adhered to. > > *not* suggest that it's the destination state. > > Adding proper docstrings should be done during mypy conversion once the > types are determined, understood, and enforced. Not before then. > >> line-too-long might be worth cleaning up. How many of them do we have >> now? >> > > Five in iotests.py using the default length of 100. 15 if I limit to 80. > > If we agree that 100 is okay, I can tackle this in an amendment patch. > If 80 is okay, I'm going to put it off as one coat of paint too many. > > (I will try to clean up the 100+ lines for my next version. I am > hesitant to make a deeper cut because I have the feeling it's the type > of series that will incur a lot of nitpicks on indent style.) One step at a time.
diff --git a/tests/qemu-iotests/pylintrc b/tests/qemu-iotests/pylintrc new file mode 100644 index 0000000000..feed506f75 --- /dev/null +++ b/tests/qemu-iotests/pylintrc @@ -0,0 +1,20 @@ +[MESSAGES CONTROL] + +# Disable the message, report, category or checker with the given id(s). You +# can either give multiple identifiers separated by comma (,) or put this +# option multiple times (only on the command line, not in the configuration +# file where it should appear only once). You can also use "--disable=all" to +# disable everything first and then reenable specific checks. For example, if +# you want to run only the similarities checker, you can use "--disable=all +# --enable=similarities". If you want to run only the classes checker, but have +# no Warning level messages displayed, use "--disable=all --enable=classes +# --disable=W". +disable=invalid-name, + missing-docstring, + line-too-long, + too-many-lines, + too-few-public-methods, + too-many-arguments, + too-many-locals, + too-many-branches, + too-many-public-methods, \ No newline at end of file
Repeatable results. run `pylint iotests.py` and you should get a pass. Signed-off-by: John Snow <jsnow@redhat.com> --- tests/qemu-iotests/pylintrc | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 tests/qemu-iotests/pylintrc