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

Added importTester #1

Closed
wants to merge 3 commits into from

Conversation

JoshuaRM
Copy link

This PR contains additions to the pull_request.py as well as an additional script importTester.py.

This PR introduces the testing of relative and absolute imports in the keras and tests directories. The message sent to the user has been edited to now receive an additional parameter (additions) which is the import testing. The message adds the first 10 files with errors for both absolute and relative (20 total), and specifies how many remain eg. 10/56. This is done by cloning the users repo, parsing the files, then deleting the cloned repo.

Please let me know if you have any questions or concerns.

Copy link
Owner

@gabrieldemarmiesse gabrieldemarmiesse left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and sorry for the delay. I added some comments. Can you check it out? Thanks.

absKeys = list(additions['absolute'].keys())[:10]
relKeys = list(additions['relative'].keys())[:10]

absolute = "The following files should not contain relative imports\n"

Choose a reason for hiding this comment

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

Actually, I'm not certain, but I believe that it's not possible to use relative imports in test files. Meaning that there is quite some code which wouldn't be needed. Could you verify this? Thanks.

absolute = "The following files should not contain relative imports\n"
for file in range(len(absKeys)):
absolute += absKeys[file] + "\n"
absolute += ("\nThis is " + str(file+1) + " out of " + len(additions['absolute']) + " files with relative imports to be changed\n")

Choose a reason for hiding this comment

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

prefer f-string formatting when possible. The docker image used is with python 3.7 so we're safe.

@@ -0,0 +1,122 @@
import os

Choose a reason for hiding this comment

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

python files should have names with uppercases. See PEP8.


return errs

def checkImports(dirPath, style):

Choose a reason for hiding this comment

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

This function name does not follow PEP8. See https://www.python.org/dev/peps/pep-0008/

@JoshuaRM
Copy link
Author

@gabrieldemarmiesse
I have made the changes requested above, as well as made some changes I believe to be beneficial. The output of this now displays 10 files in a details/summary markdown format, with the specific errors listed within the dropdown.


os.system(cmd)

absoluteOnly = import_tester.check_imports("./" + repo_id + "/keras")

Choose a reason for hiding this comment

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

The current bot actually runs only keras-contrib. So I believe that this will fail as there is no ./keras directory in keras-contrib.

@gabrieldemarmiesse
Copy link
Owner

@JoshuaRM I'm going to be less and less active in the future on keras-team/keras, because I currently use tf.keras and I don't really want to maintain a package that I don't use. Going forward I believe that the best solution for you is to fork this repo, add your modification and make a bot account of your own.

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.

None yet

2 participants