-
-
Notifications
You must be signed in to change notification settings - Fork 100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pep8 #403
Pep8 #403
Conversation
# Conflicts: # comtypes/client/__init__.py # comtypes/client/_generate.py
related to #392 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your massive contribution.
There are blank lines in early return
s, try
blocks, if
blocks, etc.
My understanding is that these are not lines that must be left blank to comply with PEP8.
I believe that, at least here, the changes should be limited to the "auto-formatted" level to reduce the differences.
Or is there a formatter that does this that I am not familiar with?
I'm using black
in my work.
I've seen that unexpected changes occurred by autopep8(maybe?) in #391.
__str__ = __unicode__ | ||
|
||
def __cmp__(self, other): | ||
if isinstance(other, GUID): | ||
return cmp(binary(self), binary(other)) | ||
return cmp(binary(self), binary(other)) # NOQA | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this blank line pep8-compliant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again code blocks as defined by pep8. a return can be considered the end of a logical section of code and thus a separation should be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically that whole method is incorrect as it should read
def __cmp__(self, other):
if isinstance(other, GUID):
res = cmp(binary(self), binary(other)) # NOQA
else:
res = -1
return res
but seeing as how the method is slated for removal anyhow it becomes a moot point. I wasn't paying attention when I added the blank line and I should have left it alone because it is going to be removed anyhow.
@@ -302,10 +330,10 @@ class _cominterface_meta(type): | |||
_com_shutting_down = False | |||
|
|||
# Creates also a POINTER type for the newly created class. | |||
def __new__(cls, name, bases, namespace): | |||
def __new__(mcs, name, bases, namespace): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that the first argument should be cls
even if it is a metaclass.
https://mail.python.org/pipermail/python-dev/2018-January/151986.html
PyCQA/pep8-naming#114
pylint-dev/pylint#2028
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the pep8 docs.
https://peps.python.org/pep-0008/#function-and-method-arguments
Always use self for the first argument to instance methods.
Always use cls for the first argument to class methods.
If a function argument’s name clashes with a reserved keyword, it is generally better to append a single trailing underscore rather than use an abbreviation or spelling corruption. Thus class_ is better than clss. (Perhaps better is to avoid such clashes by using a synonym.)
Now it defines cls as being only used in "class methods" which __new__
is not. __new__
is also not an instance method either. so mcs is the appropriate first parameter name. Flake8 was changed without reviewing the pep documentation. I am going to open an issue for it on flake8 repo to have it corrected. I believe where the issue was when it was requiring that mcs be used outside of a class method and they ended up changing it for all class types instead of enforcing the use of mcs for metaclasses only.
@@ -537,16 +571,21 @@ def _make_dispmethods(self, methods): | |||
self.__map_case__[name.lower()] = name | |||
|
|||
def __get_baseinterface_methodcount(self): | |||
"Return the number of com methods in the base interfaces" | |||
"""Return the number of com methods in the base interfaces""" | |||
itf = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recognized that the current implementation may cause itf
to be unbound.
However, I don't think the scope of this PR includes changing the implementation.
This should be addressed in a separate issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these issues show up as a result of a linter checking the code. I am correcting what the linter has stated to be problems.
@@ -435,21 +533,24 @@ def __make_interface_pointer(self, itf): | |||
# iterate over interface inheritance in reverse order to build the | |||
# virtual function table, and leave out the 'object' base class. | |||
finder = self._get_method_finder_(itf) | |||
interface = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as line 575 in comtypes/__init__.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that is removed there is the possibility of interface not being defined and it would result in a NameError. The only way interface would get defined is from the for loop and if there is nothing to iterate over the variable would not get made.
@@ -38,24 +41,29 @@ class _coclass_meta(type): | |||
# POINTER(...) type gets a __ctypes_from_outparam__ method which | |||
# will QueryInterface for the default interface: the first one on | |||
# the coclass' _com_interfaces_ list. | |||
def __new__(cls, name, bases, namespace): | |||
klass = type.__new__(cls, name, bases, namespace) | |||
def __new__(mcs, name, bases, namespace): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as line 333 in comtypes/__init__.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
look at where the method is declared. It is inside a metaclass so the use of mcs as the first parameter is correct.
|
||
# for testing | ||
# gen_dir = None | ||
import comtypes.gen # NOQA | ||
|
||
|
||
# for testing | ||
# gen_dir = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicated lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This happened when I was hammering out a merge conflict. I will fix it.
auto who? I made all of those changes manually by hand. I added the extra lines to create "code blocks" and it apart of PEP8 https://peps.python.org/pep-0008/#blank-lines
|
The whole purpose to pep8 is to make the code readable which makes it easier to maintain. look at this block of code def make_type(self, tdesc, tinfo):
# type: (typeinfo.TYPEDESC, typeinfo.ITypeInfo) -> Any
if tdesc.vt in COMTYPES:
return COMTYPES[tdesc.vt]
if tdesc.vt == automation.VT_CARRAY:
arraydesc = tdesc._.lpadesc[0] # type: typeinfo.tagARRAYDESC
typ = self.make_type(arraydesc.tdescElem, tinfo)
for i in range(arraydesc.cDims):
typ = typedesc.ArrayType(
typ, arraydesc.rgbounds[i].lLbound, arraydesc.rgbounds[i].cElements-1
)
return typ
elif tdesc.vt == automation.VT_PTR:
ptrdesc = tdesc._.lptdesc[0] # type: typeinfo.TYPEDESC
typ = self.make_type(ptrdesc, tinfo)
return PTR(typ)
elif tdesc.vt == automation.VT_USERDEFINED:
try:
ti = tinfo.GetRefTypeInfo(tdesc._.hreftype)
except COMError as details:
type_name = "__error_hreftype_%d__" % tdesc._.hreftype
tlib_name = get_tlib_filename(self.tlib)
if tlib_name is None:
tlib_name = "unknown typelib"
message = "\n\tGetRefTypeInfo failed in %s: %s\n\tgenerating type '%s' instead" % \
(tlib_name, details, type_name)
import warnings
warnings.warn(message, UserWarning);
result = typedesc.Structure(
type_name, align=8, members=[], bases=[], size=0
)
return result
result = self.parse_typeinfo(ti)
assert result is not None, ti.GetDocumentation(-1)[0]
return result
elif tdesc.vt == automation.VT_SAFEARRAY:
# SAFEARRAY(<type>), see Don Box pp.331f
safearraydesc = tdesc._.lptdesc[0] # type: typeinfo.TYPEDESC
return midlSAFEARRAY(self.make_type(safearraydesc, tinfo))
raise NotImplementedError(tdesc.vt) then look at this one def make_type(self, tdesc, tinfo):
# type: (typeinfo.TYPEDESC, typeinfo.ITypeInfo) -> Any
if tdesc.vt in COMTYPES:
return COMTYPES[tdesc.vt]
if tdesc.vt == automation.VT_CARRAY:
arraydesc = tdesc._.lpadesc[0] # type: typeinfo.tagARRAYDESC
typ = self.make_type(arraydesc.tdescElem, tinfo)
for i in range(arraydesc.cDims):
typ = typedesc.ArrayType(
typ, arraydesc.rgbounds[i].lLbound, arraydesc.rgbounds[i].cElements-1
)
return typ
elif tdesc.vt == automation.VT_PTR:
ptrdesc = tdesc._.lptdesc[0] # type: typeinfo.TYPEDESC
typ = self.make_type(ptrdesc, tinfo)
return PTR(typ)
elif tdesc.vt == automation.VT_USERDEFINED:
try:
ti = tinfo.GetRefTypeInfo(tdesc._.hreftype)
except COMError as details:
type_name = "__error_hreftype_%d__" % tdesc._.hreftype
tlib_name = get_tlib_filename(self.tlib)
if tlib_name is None:
tlib_name = "unknown typelib"
message = "\n\tGetRefTypeInfo failed in %s: %s\n\tgenerating type '%s' instead" % \
(tlib_name, details, type_name)
import warnings
warnings.warn(message, UserWarning);
result = typedesc.Structure(
type_name, align=8, members=[], bases=[], size=0
)
return result
result = self.parse_typeinfo(ti)
assert result is not None, ti.GetDocumentation(-1)[0]
return result
elif tdesc.vt == automation.VT_SAFEARRAY:
# SAFEARRAY(<type>), see Don Box pp.331f
safearraydesc = tdesc._.lptdesc[0] # type: typeinfo.TYPEDESC
return midlSAFEARRAY(self.make_type(safearraydesc, tinfo))
raise NotImplementedError(tdesc.vt) adding the single empty line makes it easier to understand. It separate 2 different things that are being done. All I did was add a blank line between sections of code that are doing different things. and to further improve the readability def make_type(self, tdesc, tinfo):
# type: (typeinfo.TYPEDESC, typeinfo.ITypeInfo) -> Any
import warnings
if tdesc.vt in COMTYPES:
res = COMTYPES[tdesc.vt]
elif tdesc.vt == automation.VT_CARRAY:
arraydesc = tdesc._.lpadesc[0] # type: typeinfo.tagARRAYDESC
typ = self.make_type(arraydesc.tdescElem, tinfo)
for i in range(arraydesc.cDims):
typ = typedesc.ArrayType(
typ,
arraydesc.rgbounds[i].lLbound,
arraydesc.rgbounds[i].cElements-1
)
res = typ
elif tdesc.vt == automation.VT_PTR:
ptrdesc = tdesc._.lptdesc[0] # type: typeinfo.TYPEDESC
typ = self.make_type(ptrdesc, tinfo)
res = PTR(typ)
elif tdesc.vt == automation.VT_USERDEFINED:
try:
ti = tinfo.GetRefTypeInfo(tdesc._.hreftype)
except COMError as details:
type_name = "__error_hreftype_%d__" % tdesc._.hreftype
tlib_name = get_tlib_filename(self.tlib)
if tlib_name is None:
tlib_name = "unknown typelib"
message = (
"\n\tGetRefTypeInfo failed in %s: %s\n\tgenerating "
"type '%s' instead" % (tlib_name, details, type_name)
)
warnings.warn(message, UserWarning)
result = typedesc.Structure(
type_name,
align=8,
members=[],
bases=[],
size=0
)
res = result
else:
res = self.parse_typeinfo(ti)
assert res is not None, ti.GetDocumentation(-1)[0]
elif tdesc.vt == automation.VT_SAFEARRAY:
# SAFEARRAY(<type>), see Don Box pp.331f
safearraydesc = tdesc._.lptdesc[0] # type: typeinfo.TYPEDESC
res = midlSAFEARRAY(self.make_type(safearraydesc, tinfo))
else:
raise NotImplementedError(tdesc.vt)
return res If the returns are to be left how they are then this would be the appropriate code def make_type(self, tdesc, tinfo):
# type: (typeinfo.TYPEDESC, typeinfo.ITypeInfo) -> Any
if tdesc.vt in COMTYPES:
return COMTYPES[tdesc.vt]
if tdesc.vt == automation.VT_CARRAY:
arraydesc = tdesc._.lpadesc[0] # type: typeinfo.tagARRAYDESC
typ = self.make_type(arraydesc.tdescElem, tinfo)
for i in range(arraydesc.cDims):
typ = typedesc.ArrayType(
typ, arraydesc.rgbounds[i].lLbound, arraydesc.rgbounds[i].cElements-1
)
return typ
if tdesc.vt == automation.VT_PTR:
ptrdesc = tdesc._.lptdesc[0] # type: typeinfo.TYPEDESC
typ = self.make_type(ptrdesc, tinfo)
return PTR(typ)
if tdesc.vt == automation.VT_USERDEFINED:
try:
ti = tinfo.GetRefTypeInfo(tdesc._.hreftype)
except COMError as details:
type_name = "__error_hreftype_%d__" % tdesc._.hreftype
tlib_name = get_tlib_filename(self.tlib)
if tlib_name is None:
tlib_name = "unknown typelib"
message = "\n\tGetRefTypeInfo failed in %s: %s\n\tgenerating type '%s' instead" % \
(tlib_name, details, type_name)
import warnings
warnings.warn(message, UserWarning);
result = typedesc.Structure(
type_name, align=8, members=[], bases=[], size=0
)
return result
result = self.parse_typeinfo(ti)
assert result is not None, ti.GetDocumentation(-1)[0]
return result
if tdesc.vt == automation.VT_SAFEARRAY:
# SAFEARRAY(<type>), see Don Box pp.331f
safearraydesc = tdesc._.lptdesc[0] # type: typeinfo.TYPEDESC
return midlSAFEARRAY(self.make_type(safearraydesc, tinfo))
raise NotImplementedError(tdesc.vt) |
as far as the mcs thing is concerned the whole idea of using class is under the assumption that everyone knows that The mailing list and all of the things you have posted regarding this still do not change the fact that pep8 has not changed at all regarding this. While Guido is the original author he is not the single controlling person. There are many people that decided changes and those people come together to make a decision. You have to look at what was said as well.
Key words are "I Think" as in a thought that it should be but there could be reasons why it shouldn't and that more looking into it should be done to make the decision that a change should be made. This is why you have not seen a change to PEP regarding this. I believe that the folks that make the linters jumped the gun and made changes to the linters that they shouldn't have. I can see both side of the coin on this but I am also looking at it from the point of view as someone that doesn't know the cpython back end and doesn't know that run the code to see what I am talking about class TestMeta(type):
def __new__(cls, *args, **kwargs):
print('TestMeta.__new__:', cls)
return super(TestMeta, cls).__new__(cls, *args, **kwargs)
def __init__(cls, name, bases, dct):
print('TestMeta.__init__:', cls)
super(TestMeta, cls).__init__(name, bases, dct)
def __call__(cls, *args, **kwargs):
print('TestMeta.__call__:', cls)
return super(TestMeta, cls).__call__(*args, **kwargs)
class Test(metaclass=TestMeta):
def __init__(self):
print('Test.__init__:', self)
def __new__(cls, *args, **kwargs):
print('Test.__new__:', cls)
return super(Test, cls).__new__(cls)
test = Test() >>> TestMeta.__new__: <class '__main__.TestMeta'>
>>> TestMeta.__init__: <class '__main__.Test'>
>>> TestMeta.__call__: <class '__main__.Test'>
>>> Test.__new__: <class '__main__.Test'>
>>> Test.__init__: <__main__.Test object at 0x0000021DD876F220>
This leads to confusion as to what cls actually is because for some methods it is the metaclass and for other methods it is not. Working under the idea of what mcs stands for which is "metaclass" then any parameter that has the metaclass passed into it should be named mcs and not cls. by doing that it makes it easier to identify what is being passed. Once could flip it the other way around so cls would conform to a classmethod being used in a normal class but what would you use as the first parameter name for This is something that needs to be brought up again and it should be defined in the PEP explicitly and not as a kind of side note. When you look at it the way that I have explained above it makes sense as to why it should be mcs and not cls. This is a really funky gray area. The reason why I changed it to mcs is because what is being passed to it is the meta class and not the class that is being built by the metaclass. |
Thank you for responses. My confusion was just the enormous number of lines and difficult to unify the viewpoints of review. Perhaps these changes should have been separated in four PR phases: "provide line breaks and spacing", "insert blank lines for code blocks", "change args of metaclasses' And my re-responses are follows: line breaks, spacing and blank linesThese are OK for me. I aware that you did it to improve readability. first arguments of metaclasses'
|
there is a commit for each file that was done. Go through each file one at a time. |
work each commit one at a time and I can adjust as needed. The renaming of cls to mcs doesn't affect functionality and is actually a matter of preference instead of function and I will change that back as it is not a big deal. The unbound locals does have the potential to cause problems but it does fall more into line with being a bug and it should be handled in it's own PR so changing those back is the right thing to do. I did forget to mention.. don't merge this yet. I am not done with updating the files. |
OK. I am waiting for updating. If you are done, please re-request review or mentions. |
Little more things... Whenever possible, I'm going to use 1 to 2 hours per day to work on the The reviewer and the reviewee might be exhausted when they have to check multiple files from multiple viewpoints on a single PR, as in this case. Of course, I recognize that there are times when a large amount of changes must be submitted at once.
I hope that your deep technical knowledge will spread to this community and make this package even better. Please keep up the good work. |
so instead you would rather have a crap load of PRs to review? It makes ZERO difference if the changes are in a single PR or if it is broken up into 1000000 PRs. it's still the same work to review them. This PR is broken apart by file. Each commit is a file that has been updated. Click on each commit and it will show you what has changed in that file. |
Thank you for your response.
No. Aside from being an very extreme example, there is a big difference between "1 PR with 1,000,000 parts of changes" and "1,000,000 PRs with 1 part of changes for each". If it is indeed the case that too-small changes are so numerous, perhaps the problem is that the changes are not cohesive. If you say "see each commit", then you should have submitted your PR on a per-commit or per-file basis. The problem with this PR is a mixture of welcome things like line breaks and inserting blank lines for better readability, and controversial things that are different from the PEP8 recommendations even if it titled "Pep8". I can sort of see that there might be nothing wrong with your code, but the sheer volume of it makes me nervous that I am missing something. Sorry for the wall of text. |
Are you preparing or have you begun work on readability improvements instead of this PR? |
Parts of these commits should be great and I am going to cherry-pick and merge them. |
remove those commits. I have also contacted github support about you exploiting a vulnerability to add code for which you have no permission to do. |
Ok, I will remove those. |
Anyway, I should have gotten an approval from you before proceeding. My apologies. |
as the title says PEP8 changes.
makes the code easier to read.