Skip to content
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

Add check for print statement #150

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

mzfr
Copy link
Contributor

@mzfr mzfr commented Dec 10, 2018

Fixes #19

@mzfr
Copy link
Contributor Author

mzfr commented Jan 19, 2019

@Rechi I have used regex to find the print statement and I have tested this with the following content in a file.

"""
This is docstings and here is the print("testing")
"""

print("testing separately")

someclasss.print("with some function") # comment with print("in a comment")
# comment with print("in a comment separately")

and the output was print("testing separately") only

@Rechi
Copy link
Member

Rechi commented Jan 21, 2019

The following isn't detected.

def printThat(that):
    print(that)

@Rechi
Copy link
Member

Rechi commented Jan 22, 2019

if True: print('ABC') isn't detected.

@mzfr
Copy link
Contributor Author

mzfr commented Feb 13, 2019

@Rechi That would just make it's difficult to catch because if I remove the ^ from the regex pattern then it would start counting all the print statements including the comments etc

@Rechi
Copy link
Member

Rechi commented Feb 13, 2019

Doing the search for print with reg-ex is the wrong approach. You have to use some parsers which correctly understands the python syntax.

@mzfr
Copy link
Contributor Author

mzfr commented Feb 13, 2019

@Rechi I did try with AST but again that didn't covered all the cases

@mzfr
Copy link
Contributor Author

mzfr commented Mar 22, 2019

@Rechi So I tried again with the ast and I figured it out. So Now it should report any print statement ignoring the docstrings and comments.

Copy link
Member

@Rechi Rechi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

README.md update is missing.

kodi_addon_checker/check_files.py Outdated Show resolved Hide resolved
kodi_addon_checker/check_files.py Outdated Show resolved Hide resolved
kodi_addon_checker/check_addon.py Outdated Show resolved Hide resolved
kodi_addon_checker/check_files.py Outdated Show resolved Hide resolved
kodi_addon_checker/check_files.py Outdated Show resolved Hide resolved
Copy link
Member

@Rechi Rechi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updating README.md is missing

if os.path.splitext(file["name"])[1] == '.py':
file = os.path.join(file["path"], file["name"])

with open(file, 'r', encoding="utf-8") as f:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

open may raise a UnicodeDecodeError.


if node.value.func.id == "print":
report.add(Record(WARNING, "%s has a print statement on line %s" %
(relative_path(str(file)), node.line)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

node.line is wrong, is may not exist. Correct is node.lineno.

@mzfr mzfr force-pushed the print-statements branch 2 times, most recently from 363fc82 to 57f1800 Compare April 28, 2019 02:54
@Rechi
Copy link
Member

Rechi commented Aug 6, 2019

IMO print statements shouldn't be reported for script.module add-ons, as most of those are not specifically coded for Kodi.

@enen92 @razzeee what's your opinion?

@enen92
Copy link
Member

enen92 commented Aug 6, 2019

@Rechi agreed. Also not for any file containing test or within a test folder.

@Rechi
Copy link
Member

Rechi commented Aug 6, 2019

Yes test should be ignored too, but not explicit by this check. handle_files.create_file_index shouldn't return test files.

@mzfr
Copy link
Contributor Author

mzfr commented Aug 23, 2019

@Rechi Why are we ignoring the test files? I mean the @MartijnKaijser comment about people using print statement in unit test make sense.

@Rechi
Copy link
Member

Rechi commented Aug 24, 2019

Your second sentence already answers the question. In unit tests one can't use xbmc.log(...), because they aren't from inside Kodi. If tests aren't ignored you could get lot of print used reports for those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Search for print statements and report them
3 participants