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 lint for scripts and fix all issues #61

Merged
merged 6 commits into from Aug 20, 2018
Merged

Add lint for scripts and fix all issues #61

merged 6 commits into from Aug 20, 2018

Conversation

@c-w
Copy link
Contributor

@c-w c-w commented Aug 17, 2018

Also: move datafiles in common to blob storage.

@c-w c-w requested a review from armanrahman22 Aug 17, 2018
@c-w c-w force-pushed the lint-scripts branch from fe6249a to df285d6 Aug 17, 2018
Makefile Outdated

.PHONY: flake8-scripts
flake8-scripts: build-scripts
docker run --entrypoint=sh "$(get_famous_people_list)" -c "pip -qqq install flake8 && flake8 *.py"
Copy link
Collaborator

@michaelperel michaelperel Aug 20, 2018

With -qqq, is it advisable to silence critical level logging warnings?

Copy link
Contributor Author

@c-w c-w Aug 20, 2018

That should be fine. pip install is not expected to fail and if it does, the entire command will fail (due to the &&) so we know what to look into. Having low verbosity definitely is nicer in the CI run since otherwise the pip logs just drown out the lint messages.

Copy link
Contributor Author

@c-w c-w Aug 20, 2018

Reduced to -qq since that's also still non-verbose in 9274c8b. Thanks for the pointer.

@@ -0,0 +1,36 @@
from processor import normalize_images
Copy link
Collaborator

@michaelperel michaelperel Aug 20, 2018

Should we add typing to these files?

Copy link
Contributor Author

@c-w c-w Aug 20, 2018

Done in db89e46.

@c-w c-w force-pushed the lint-scripts branch from 9274c8b to 8c646cc Aug 20, 2018
@c-w c-w merged commit 6c2e169 into master Aug 20, 2018
2 checks passed
@c-w c-w deleted the lint-scripts branch Aug 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants