-
Notifications
You must be signed in to change notification settings - Fork 76
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
green_mode_core.py: Remove unnecessary import #331
base: master
Are you sure you want to change the base?
Conversation
Travis tests have failedHey @Ishaan29, 1st Buildpytest
TravisBuddy Request Identifier: 86e8cde0-11a9-11e9-a449-0b9a49f18a3c |
@@ -50,8 +50,7 @@ def green_mode(project_dir: str, ignore_globs, bears, bear_settings_obj, | |||
The maximum number of values to run the bear again and again for | |||
a optional setting. | |||
""" | |||
from coala_quickstart.green_mode.filename_operations import ( | |||
check_filename_prefix_postfix) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary newline, please remove this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the reason why travis test was failing ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build is failing due to a different reason, most probably.
Travis tests have failedHey @Ishaan29, 1st Buildpytest
TravisBuddy Request Identifier: 14e87c70-11c9-11e9-a449-0b9a49f18a3c |
I don't understand why travis-ci test 1 is failing , it worked fine on my local machine . am I missing something ?? @KVGarg @palash25 @li-boxuan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look fine but you need to squash the 2 commits into one. Checkout the Newcomers' Guide. That explains all this.
Also change .. it was already been called once , hence delayed import was unnecessary.
to .. it shadowed an identical import in the module scope.
.
Also change Fix
to Remove
in the header.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And don't worry about the travis-tests for now. If this import was actually unnecessary (which it was), the builds won't be affected.
Sure , I will implement the suggested changes |
Travis tests have failedHey @Ishaan29, 1st Buildpytest
TravisBuddy Request Identifier: 64cd8c20-1265-11e9-962f-bf63ffca8115 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!! :) Please squash your commits. Refer this
No need to worry about Travis Failure. The failure is unrelated to your changes :D
Done ! squashing commits :) |
Travis tests have failedHey @Ishaan29, 1st Buildpytest
TravisBuddy Request Identifier: b9494ae0-1315-11e9-9a38-0181ea675062 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 4th point of Commit Body section of the Commit Guidelines says the following:
First person should not be used here.
This is not being followed in this commit message.
Do you want me to change Removed -> Remove in commit message body ? |
Travis tests have failedHey @Ishaan29, 1st Buildpytest
TravisBuddy Request Identifier: e3472c20-134e-11e9-9a38-0181ea675062 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit message still needs changes. Please check out the examples in the Commit Guidelines. It should start with something like This removes ...
This removes delayed import of function check_filename_prefix_postfix as it shadowed an identical import in module scope Closes coala#300
Travis tests have failedHey @Ishaan29, 1st Buildpytest
TravisBuddy Request Identifier: f21d3230-1411-11e9-be9a-0de216b0fb54 |
changed the commit message as per the commit message guideline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat. LGTM.
However, you could put the title, the same as the commit head ;)
Sure @utkarsh2102 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be mergable when it is rebased.
Is this PR mergable ? |
Removed the delayed import of function check_filename_prefix_postfix
as it was already been called once , hence delayed import was
unnecessary
Closes #300