Skip to content

[BEAM-2000] run pylint on specific module#2579

Closed
ubunatic wants to merge 4 commits intoapache:masterfrom
ubunatic:run-pylint-module-option
Closed

[BEAM-2000] run pylint on specific module#2579
ubunatic wants to merge 4 commits intoapache:masterfrom
ubunatic:run-pylint-module-option

Conversation

@ubunatic
Copy link

  • added module option: any given args to the script will be used as the module for pep8 and pylint
  • cleaned up bash syntax: removed square brackets, using more explicit test command instead

@aaltay
Copy link
Member

aaltay commented Apr 18, 2017

R: @sb2nov

@coveralls
Copy link

Coverage Status

Coverage remained the same at 70.669% when pulling 8a3f9a7 on ubunatic:run-pylint-module-option into 8f4bb01 on apache:master.

@sb2nov
Copy link
Contributor

sb2nov commented Apr 18, 2017

Thanks, looks good apart from the two nits.

if [[ $FILES_TO_IGNORE ]]; then
FILES_TO_IGNORE="$FILES_TO_IGNORE, "
if test -z "$FILES_TO_IGNORE"
then FILES_TO_IGNORE="$(basename $file)"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Indent the then & else inside the if

Copy link
Author

Choose a reason for hiding this comment

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

OK.
However, I think the non-indented then and else is more readable in bash in this case, i.e., you should write either:

if ...
then single command
else single command
fi

or for multiple commands:

if ...; then
   indented commands
   more commands
else
   indented command
   more commands
fi

echo "Running pep8:"
pep8 apache_beam --exclude="$FILES_TO_IGNORE"
if test $# -gt 0
then MODULE="$@"
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Indent the then & else inside the if

Copy link
Author

Choose a reason for hiding this comment

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

Done

@sb2nov
Copy link
Contributor

sb2nov commented Apr 18, 2017

LGTM, thanks for this 👍

@aaltay please merge

@aaltay
Copy link
Member

aaltay commented Apr 18, 2017

Could you add a comment to the top of the file on how to use this (something like ./run_pylint.sh [module], module defaults apache_beam)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 70.456% when pulling a98fe93 on ubunatic:run-pylint-module-option into 8f4bb01 on apache:master.

@ubunatic
Copy link
Author

ubunatic commented Apr 19, 2017

OK I added the usage info.

Other question:
Did I miss any steps to make the CI checks succeed?
If they fail for most/all PRs, shouldn't we probably disable or fix them first?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.003%) to 70.131% when pulling 926eb08 on ubunatic:run-pylint-module-option into 8319369 on apache:master.

@aaltay
Copy link
Member

aaltay commented Apr 19, 2017

LGTM.

Travis tests are being currently removed. Although Travis tests give valid signals for Python development they are taking too long and timing out for Java tests. Part of the reason they are still there is Python on Mac tests are useful. (@jasonkuster may have more details)

Jenkins tests should not fail. If they do for an unrelated reason (like in this case) please file an issue for a flaky/failing tests.

@ubunatic
Copy link
Author

Thx for the info. The Jenkins test failed because of the following flaky test https://issues.apache.org/jira/browse/BEAM-1868

@sb2nov
Copy link
Contributor

sb2nov commented Apr 19, 2017

Retest this please

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.006%) to 70.128% when pulling 926eb08 on ubunatic:run-pylint-module-option into 8319369 on apache:master.

@asfgit asfgit closed this in 3ef614c Apr 19, 2017
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.

4 participants