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.

Sign up for GitHub

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

Jump to bottom

Pylint errors does not allow pre_push_hook to fail #6040

Closed
lilithxxx opened this issue Dec 30, 2018 · 5 comments
Closed

Pylint errors does not allow pre_push_hook to fail #6040

lilithxxx opened this issue Dec 30, 2018 · 5 comments

Comments

@lilithxxx
Copy link
Contributor

lilithxxx commented Dec 30, 2018 •
edited
Loading

In case of a Pylint error the pre_push_hook should fail. This is not observed currently as the error code returned from the pre_commit_linter in case of a Pylint error is zero (should be non-zero).

Following is an example:

> python2 scripts/pre_commit_linter.py; echo $?
Starting directive scope check
----------------------------------------
SUCCESS  Directive scope check passed

----------------------------------------

Starting controller dependency line break check
----------------------------------------
SUCCESS  Controller dependency line break check passed

----------------------------------------

Starting HTML directive name check
----------------------------------------

----------------------------------------

There are no files to be checked.
Starting import-order checks
----------------------------------------

----------------------------------------

Starting newline-at-EOF checks
----------------------------------------

----------------------------------------

(1 files checked, 0 errors found)
SUCCESS   Newline character checks passed
Starting docstring checks
----------------------------------------

----------------------------------------

SUCCESS   Docstring check passed
Starting comment checks
----------------------------------------

----------------------------------------

SUCCESS   Comments check passed
Starting HTML tag and attribute check
----------------------------------------
SUCCESS  HTML tag and attribute check passed

----------------------------------------

Starting HTML linter...
----------------------------------------

----------------------------------------

SUCCESS   HTML linting passed
HTML linting finished.

Starting linter...
Starting CSS, Javascript and Python Linting
----------------------------------------
There are no CSS files to lint.
There are no CSS files to lint.
There are no JavaScript files to lint.
Linting 1 Python files
Linting Python files 1 to 1...
Using config file /home/jude/foss/oppia/.pylintrc
Python linting finished.


Summary of Errors:
----------------------------------------



************* Module oppia.scripts.pre_commit_linter
W:467, 4: Unused variable 'temp' (unused-variable)

------------------------------------------------------------------
Your code has been rated at 9.99/10 (previous run: 9.99/10, +0.00)



Starting Pattern Checks
----------------------------------------

----------------------------------------

There are no files to be checked.
Starting copyright notice check
----------------------------------------
SUCCESS  Copyright notice check passed

----------------------------------------

0

The last line denotes the status code which should be non-zero.
@1995YogeshSharma
Copy link
Contributor

Are you sure?
In pre_commit_linter.py here, the returned status is 1 if there is any failed message and it is handled in pre_push_hook.py

@lilithxxx
Copy link
Contributor Author

@1995YogeshSharma Yes the pre_commit_linter returns status code 1 when the functions defined in the pre_commit_linter itself fails but does not take those into account which rises due to pylint.

@apb7
Copy link
Contributor

apb7 commented Dec 30, 2018

@lilithxxx @1995YogeshSharma: We have PR #6038 for this. This is happening due to the way we are handling queues. You can find further details here: #6038 (comment).
Thanks!

@lilithxxx
Copy link
Contributor Author

@apb7 Ah the PR does seem to fix this. Should I close this issue then?

@apb7
Copy link
Contributor

apb7 commented Dec 30, 2018

@lilithxxx, please let this issue remain open. We'll close it when the PR goes in. Thanks for the issue :)

@ankita240796 ankita240796 mentioned this issue Jan 1, 2019
Merged
@apb7 apb7 closed this as completed in #6038 Jan 1, 2019
apb7 pushed a commit that referenced this issue Jan 1, 2019
@ankita240796 @apb7
Fix #6040: Fix issues in develop (#6038)
59260be 
* Fix issues in develop

* revert changes

* Increase value of timeout_multiplier

* Linter catches the issues, redo the changes

* Revert changes in exp services test

* Revert changes in top navigation bar directive

* Update method to obtain messages to fix linter

* Address review comments

* Fix issues

* Increase timeout

* Increase timeout

* Revert changes

* Revert changes

* Increase timeout further

* Fix pylint error

* Fix js errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@1995YogeshSharma @apb7 @lilithxxx