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 mzfr force-pushed the print-statements branch from 49820af to b8e49e4 Compare January 19, 2019 08:42
@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

@mzfr mzfr force-pushed the print-statements branch from b8e49e4 to 3b2113a Compare January 20, 2019 13:32
@Rechi
Copy link
Member

Rechi commented Jan 21, 2019

The following isn't detected.

def printThat(that):
    print(that)

@mzfr mzfr force-pushed the print-statements branch from 3b2113a to efffedf Compare January 21, 2019 13:38
@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 mzfr force-pushed the print-statements branch from efffedf to b64dfb4 Compare March 22, 2019 08:14
@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
@mzfr mzfr force-pushed the print-statements branch from b64dfb4 to 0d349eb Compare March 29, 2019 15:22
kodi_addon_checker/check_files.py Outdated Show resolved Hide resolved
kodi_addon_checker/check_files.py Outdated Show resolved Hide resolved
@mzfr mzfr force-pushed the print-statements branch from 0d349eb to 1af0d82 Compare April 13, 2019 20:12
@mzfr mzfr force-pushed the print-statements branch from 1af0d82 to cc2237d Compare April 20, 2019 12:44
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
@mzfr mzfr force-pushed the print-statements branch from 57f1800 to 13b1ba9 Compare August 3, 2019 05:44
@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.

@Rechi Rechi force-pushed the print-statements branch from e187c8e to 890afd7 Compare July 3, 2023 19:08
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